| Message ID | 20180827093444.23623-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7] Add udmabuf misc device | expand |
On Mon, Aug 27, 2018 at 11:34:44AM +0200, Gerd Hoffmann wrote: > A driver to let userspace turn memfd regions 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. > > qemu test branch: > https://git.kraxel.org/cgit/qemu/log/?h=sirius/udmabuf > > Cc: David Airlie <airlied@linux.ie> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > Documentation/ioctl/ioctl-number.txt | 1 + > include/uapi/linux/udmabuf.h | 33 +++ > drivers/dma-buf/udmabuf.c | 287 ++++++++++++++++++++++ > tools/testing/selftests/drivers/dma-buf/udmabuf.c | 96 ++++++++ > MAINTAINERS | 16 ++ > drivers/dma-buf/Kconfig | 8 + > drivers/dma-buf/Makefile | 1 + > tools/testing/selftests/drivers/dma-buf/Makefile | 5 + > 8 files changed, 447 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/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt > index 13a7c999c0..f2ac672eb7 100644 > --- a/Documentation/ioctl/ioctl-number.txt > +++ b/Documentation/ioctl/ioctl-number.txt > @@ -272,6 +272,7 @@ Code Seq#(hex) Include File Comments > 't' 90-91 linux/toshiba.h toshiba and toshiba_acpi SMM > 'u' 00-1F linux/smb_fs.h gone > 'u' 20-3F linux/uvcvideo.h USB video class host driver > +'u' 40-4f linux/udmabuf.h userspace dma-buf misc device > 'v' 00-1F linux/ext2_fs.h conflict! > 'v' 00-1F linux/fs.h conflict! > 'v' 00-0F linux/sonypi.h conflict! > diff --git a/include/uapi/linux/udmabuf.h b/include/uapi/linux/udmabuf.h > new file mode 100644 > index 0000000000..46b6532ed8 > --- /dev/null > +++ b/include/uapi/linux/udmabuf.h > @@ -0,0 +1,33 @@ > +/* 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> > + > +#define UDMABUF_FLAGS_CLOEXEC 0x01 > + > +struct udmabuf_create { > + __u32 memfd; > + __u32 flags; > + __u64 offset; > + __u64 size; > +}; > + > +struct udmabuf_create_item { > + __u32 memfd; > + __u32 __pad; > + __u64 offset; > + __u64 size; > +}; > + > +struct udmabuf_create_list { > + __u32 flags; > + __u32 count; > + struct udmabuf_create_item list[]; > +}; > + > +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create) > +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct udmabuf_create_list) > + > +#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..8e24204526 > --- /dev/null > +++ b/drivers/dma-buf/udmabuf.c > @@ -0,0 +1,287 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#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 <linux/shmem_fs.h> > +#include <linux/memfd.h> > + > +#include <uapi/linux/udmabuf.h> > + > +struct udmabuf { > + u32 pagecount; > + struct page **pages; > +}; > + > +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 | VM_MAYSHARE)) == 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]); > + kfree(ubuf->pages); > + kfree(ubuf); > +} > + > +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 void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num, > + void *vaddr) > +{ > + kunmap(vaddr); > +} > + > +static struct dma_buf_ops udmabuf_ops = { > + .map_dma_buf = map_udmabuf, > + .unmap_dma_buf = unmap_udmabuf, > + .release = release_udmabuf, > + .map = kmap_udmabuf, > + .unmap = kunmap_udmabuf, > + .mmap = mmap_udmabuf, > +}; > + > +#define SEALS_WANTED (F_SEAL_SHRINK) > +#define SEALS_DENIED (F_SEAL_WRITE) > + > +static long udmabuf_create(struct udmabuf_create_list *head, > + struct udmabuf_create_item *list) > +{ > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > + struct file *memfd = NULL; > + struct udmabuf *ubuf; > + struct dma_buf *buf; > + pgoff_t pgoff, pgcnt, pgidx, pgbuf; > + struct page *page; > + int seals, ret = -EINVAL; > + u32 i, flags; > + > + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); > + if (!ubuf) > + return -ENOMEM; > + > + for (i = 0; i < head->count; i++) { > + if (!IS_ALIGNED(list[i].offset, PAGE_SIZE)) > + goto err_free_ubuf; > + if (!IS_ALIGNED(list[i].size, PAGE_SIZE)) > + goto err_free_ubuf; > + ubuf->pagecount += list[i].size >> PAGE_SHIFT; > + } > + ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page *), > + GFP_KERNEL); > + if (!ubuf->pages) { > + ret = -ENOMEM; > + goto err_free_ubuf; > + } > + > + pgbuf = 0; > + for (i = 0; i < head->count; i++) { > + memfd = fget(list[i].memfd); > + if (!memfd) > + goto err_put_pages; > + if (!shmem_mapping(file_inode(memfd)->i_mapping)) > + goto err_put_pages; > + seals = memfd_fcntl(memfd, F_GET_SEALS, 0); > + if (seals == -EINVAL || > + (seals & SEALS_WANTED) != SEALS_WANTED || > + (seals & SEALS_DENIED) != 0) > + goto err_put_pages; > + pgoff = list[i].offset >> PAGE_SHIFT; > + pgcnt = list[i].size >> PAGE_SHIFT; > + for (pgidx = 0; pgidx < pgcnt; pgidx++) { > + page = shmem_read_mapping_page( > + file_inode(memfd)->i_mapping, pgoff + pgidx); > + if (IS_ERR(page)) { > + ret = PTR_ERR(page); > + goto err_put_pages; > + } > + ubuf->pages[pgbuf++] = page; > + } > + fput(memfd); > + } > + memfd = NULL; > + > + 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 (head->flags & UDMABUF_FLAGS_CLOEXEC) > + flags |= O_CLOEXEC; > + return dma_buf_fd(buf, flags); > + > +err_put_pages: > + while (pgbuf > 0) > + put_page(ubuf->pages[--pgbuf]); > +err_free_ubuf: > + fput(memfd); > + kfree(ubuf->pages); > + kfree(ubuf); > + return ret; > +} > + > +static long udmabuf_ioctl_create(struct file *filp, unsigned long arg) > +{ > + struct udmabuf_create create; > + struct udmabuf_create_list head; > + struct udmabuf_create_item list; > + > + if (copy_from_user(&create, (void __user *)arg, > + sizeof(struct udmabuf_create))) > + return -EFAULT; > + > + head.flags = create.flags; > + head.count = 1; > + list.memfd = create.memfd; > + list.offset = create.offset; > + list.size = create.size; > + > + return udmabuf_create(&head, &list); > +} > + > +static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg) > +{ > + struct udmabuf_create_list head; > + struct udmabuf_create_item *list; > + int ret = -EINVAL; > + u32 lsize; > + > + if (copy_from_user(&head, (void __user *)arg, sizeof(head))) > + return -EFAULT; > + if (head.count > 1024) > + return -EINVAL; > + lsize = sizeof(struct udmabuf_create_item) * head.count; > + list = memdup_user((void __user *)(arg + sizeof(head)), lsize); > + if (IS_ERR(list)) > + return PTR_ERR(list); > + > + ret = udmabuf_create(&head, list); > + kfree(list); > + 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; > + case UDMABUF_CREATE_LIST: > + ret = udmabuf_ioctl_create_list(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@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..376b1d6730 > --- /dev/null > +++ b/tools/testing/selftests/drivers/dma-buf/udmabuf.c > @@ -0,0 +1,96 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#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 <sys/syscall.h> > +#include <linux/memfd.h> > +#include <linux/udmabuf.h> > + > +#define TEST_PREFIX "drivers/dma-buf/udmabuf" > +#define NUM_PAGES 4 > + > +static int memfd_create(const char *name, unsigned int flags) > +{ > + return syscall(__NR_memfd_create, name, flags); > +} > + > +int main(int argc, char *argv[]) > +{ > + struct udmabuf_create create; > + int devfd, memfd, buf, ret; > + off_t size; > + void *mem; > + > + devfd = open("/dev/udmabuf", O_RDWR); > + if (devfd < 0) { > + printf("%s: [skip,no-udmabuf]\n", TEST_PREFIX); > + exit(77); > + } > + > + memfd = memfd_create("udmabuf-test", MFD_CLOEXEC); > + if (memfd < 0) { > + printf("%s: [skip,no-memfd]\n", TEST_PREFIX); > + exit(77); > + } > + > + size = getpagesize() * NUM_PAGES; > + ret = ftruncate(memfd, size); > + if (ret == -1) { > + printf("%s: [FAIL,memfd-truncate]\n", TEST_PREFIX); > + exit(1); > + } > + > + memset(&create, 0, sizeof(create)); > + > + /* should fail (offset not page aligned) */ > + create.memfd = memfd; > + create.offset = getpagesize()/2; > + create.size = getpagesize(); > + buf = ioctl(devfd, UDMABUF_CREATE, &create); > + if (buf >= 0) { > + printf("%s: [FAIL,test-1]\n", TEST_PREFIX); > + exit(1); > + } > + > + /* should fail (size not multiple of page) */ > + create.memfd = memfd; > + create.offset = 0; > + create.size = getpagesize()/2; > + buf = ioctl(devfd, UDMABUF_CREATE, &create); > + if (buf >= 0) { > + printf("%s: [FAIL,test-2]\n", TEST_PREFIX); > + exit(1); > + } > + > + /* should fail (not memfd) */ > + create.memfd = 0; /* stdin */ > + create.offset = 0; > + create.size = size; > + buf = ioctl(devfd, UDMABUF_CREATE, &create); > + if (buf >= 0) { > + printf("%s: [FAIL,test-3]\n", TEST_PREFIX); > + exit(1); > + } > + > + /* should work */ > + create.memfd = memfd; > + create.offset = 0; > + create.size = size; > + buf = ioctl(devfd, UDMABUF_CREATE, &create); > + if (buf < 0) { > + printf("%s: [FAIL,test-4]\n", TEST_PREFIX); > + exit(1); > + } > + > + fprintf(stderr, "%s: ok\n", TEST_PREFIX); > + close(buf); > + close(memfd); > + close(devfd); > + return 0; > +} > diff --git a/MAINTAINERS b/MAINTAINERS > index a5b256b259..11a9b04277 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14934,6 +14934,14 @@ S: Maintained > F: Documentation/filesystems/udf.txt > F: fs/udf/ > > +UDMABUF DRIVER > +M: Gerd Hoffmann <kraxel@redhat.com> > +L: dri-devel@lists.freedesktop.org > +S: Maintained > +F: drivers/dma-buf/udmabuf.c > +F: include/uapi/linux/udmabuf.h > +F: tools/testing/selftests/drivers/dma-buf/udmabuf.c Dupe, remove this one (since only the 2nd one has the drm-misc git repo entry). Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + > UDRAW TABLET > M: Bastien Nocera <hadess@hadess.net> > L: linux-input@vger.kernel.org > @@ -15343,6 +15351,14 @@ F: arch/x86/um/ > F: fs/hostfs/ > F: fs/hppfs/ > > +USERSPACE DMA BUFFER DRIVER > +M: Gerd Hoffmann <kraxel@redhat.com> > +S: Maintained > +L: dri-devel@lists.freedesktop.org > +F: drivers/dma-buf/udmabuf.c > +F: include/uapi/linux/udmabuf.h > +T: git git://anongit.freedesktop.org/drm/drm-misc > + > USERSPACE I/O (UIO) > M: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > S: Maintained > diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig > index ed3b785bae..338129eb12 100644 > --- a/drivers/dma-buf/Kconfig > +++ b/drivers/dma-buf/Kconfig > @@ -30,4 +30,12 @@ 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 memfd regions into dma-bufs. > + Qemu can use this to create host dmabufs for guest framebuffers. > + > 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 >
Hi Gerd, Thank you for the patch. CC'ing the linux-api mailing list as this creates a new userspace API. On Monday, 27 August 2018 12:34:44 EEST Gerd Hoffmann wrote: > A driver to let userspace turn memfd regions 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. > > qemu test branch: > https://git.kraxel.org/cgit/qemu/log/?h=sirius/udmabuf > > Cc: David Airlie <airlied@linux.ie> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > Documentation/ioctl/ioctl-number.txt | 1 + > include/uapi/linux/udmabuf.h | 33 +++ > drivers/dma-buf/udmabuf.c | 287 +++++++++++++++++++ > tools/testing/selftests/drivers/dma-buf/udmabuf.c | 96 ++++++++ > MAINTAINERS | 16 ++ > drivers/dma-buf/Kconfig | 8 + > drivers/dma-buf/Makefile | 1 + > tools/testing/selftests/drivers/dma-buf/Makefile | 5 + > 8 files changed, 447 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 [snip] > diff --git a/include/uapi/linux/udmabuf.h b/include/uapi/linux/udmabuf.h > new file mode 100644 > index 0000000000..46b6532ed8 > --- /dev/null > +++ b/include/uapi/linux/udmabuf.h > @@ -0,0 +1,33 @@ > +/* 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> > + > +#define UDMABUF_FLAGS_CLOEXEC 0x01 > + > +struct udmabuf_create { > + __u32 memfd; > + __u32 flags; > + __u64 offset; > + __u64 size; > +}; > + > +struct udmabuf_create_item { > + __u32 memfd; > + __u32 __pad; > + __u64 offset; > + __u64 size; > +}; > + > +struct udmabuf_create_list { > + __u32 flags; > + __u32 count; > + struct udmabuf_create_item list[]; > +}; > + > +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create) Why do you start at 0x42 if you reserve the 0x40-0x4f range ? > +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct udmabuf_create_list) Where's the documentation ? :-) > +#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..8e24204526 > --- /dev/null > +++ b/drivers/dma-buf/udmabuf.c > @@ -0,0 +1,287 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#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 <linux/shmem_fs.h> > +#include <linux/memfd.h> Could you please keep the #include alphabetically sorted ? It helps locating duplicates. > +#include <uapi/linux/udmabuf.h> I think you can just #include <linux/udmabuf.h>, no need to use the uapi/ prefix. > +struct udmabuf { > + u32 pagecount; > + struct page **pages; > +}; > + > +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; Just curious, when do you expect this to happen ? > + 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 | VM_MAYSHARE)) == 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; You can return ERR_PTR(-ENOMEM) directly. > + if (sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount, > + 0, ubuf->pagecount << PAGE_SHIFT, > + GFP_KERNEL) < 0) Shouldn't you propagate the return value from sg_alloc_table_from_pages() ? > + 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); You can merge all these labels with error: if (sg) { sg_free_table(sg); kfree(sg); } 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++) Shouldn't both pg and pagecount have the same type ? Why does the loop counter qualify for a pgoff_t and not the pagecount field ? Granted, the pgoff_t is documented as "The type of an index in the page cache", so pagecount doesn't really quality, but the fact that one is an unsigned long and the other a u32 makes me think that something is wrong. > + put_page(ubuf->pages[pg]); > + kfree(ubuf->pages); > + kfree(ubuf); > +} > + > +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 void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num, > + void *vaddr) > +{ > + kunmap(vaddr); > +} > + > +static struct dma_buf_ops udmabuf_ops = { static const struct > + .map_dma_buf = map_udmabuf, > + .unmap_dma_buf = unmap_udmabuf, > + .release = release_udmabuf, > + .map = kmap_udmabuf, > + .unmap = kunmap_udmabuf, > + .mmap = mmap_udmabuf, > +}; > + > +#define SEALS_WANTED (F_SEAL_SHRINK) > +#define SEALS_DENIED (F_SEAL_WRITE) > + > +static long udmabuf_create(struct udmabuf_create_list *head, > + struct udmabuf_create_item *list) Those two structures are not modified by the function, you can make them const. > +{ > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > + struct file *memfd = NULL; > + struct udmabuf *ubuf; > + struct dma_buf *buf; > + pgoff_t pgoff, pgcnt, pgidx, pgbuf; > + struct page *page; > + int seals, ret = -EINVAL; > + u32 i, flags; > + > + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); sizeof(*ubuf) > + if (!ubuf) > + return -ENOMEM; > + > + for (i = 0; i < head->count; i++) { > + if (!IS_ALIGNED(list[i].offset, PAGE_SIZE)) > + goto err_free_ubuf; > + if (!IS_ALIGNED(list[i].size, PAGE_SIZE)) > + goto err_free_ubuf; > + ubuf->pagecount += list[i].size >> PAGE_SHIFT; Is there a risk of overflowing pagecount ? > + } > + ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page *), > + GFP_KERNEL); sizeof(*ubuf->pages) > + if (!ubuf->pages) { > + ret = -ENOMEM; > + goto err_free_ubuf; > + } > + > + pgbuf = 0; > + for (i = 0; i < head->count; i++) { > + memfd = fget(list[i].memfd); > + if (!memfd) > + goto err_put_pages; > + if (!shmem_mapping(file_inode(memfd)->i_mapping)) > + goto err_put_pages; > + seals = memfd_fcntl(memfd, F_GET_SEALS, 0); > + if (seals == -EINVAL || > + (seals & SEALS_WANTED) != SEALS_WANTED || > + (seals & SEALS_DENIED) != 0) > + goto err_put_pages; All these conditions will return -EINVAL. I'm not familiar with the memfd API, should some error conditions return a different error code to make them distinguishable by userspace ? > + pgoff = list[i].offset >> PAGE_SHIFT; > + pgcnt = list[i].size >> PAGE_SHIFT; > + for (pgidx = 0; pgidx < pgcnt; pgidx++) { > + page = shmem_read_mapping_page( > + file_inode(memfd)->i_mapping, pgoff + pgidx); Can't pgoff + pgcnt overflow the total number of avialble pages ? > + if (IS_ERR(page)) { > + ret = PTR_ERR(page); > + goto err_put_pages; > + } > + ubuf->pages[pgbuf++] = page; > + } > + fput(memfd); > + } > + memfd = NULL; I'd move this line just after fput(memfd) inside the loop to avoid introduction bugs in the future if we add code that can break from the loop before the fget() call. > + 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 (head->flags & UDMABUF_FLAGS_CLOEXEC) > + flags |= O_CLOEXEC; > + return dma_buf_fd(buf, flags); > + > +err_put_pages: > + while (pgbuf > 0) > + put_page(ubuf->pages[--pgbuf]); If you initialize pgbuf to 0 you can merge the two error labels. > +err_free_ubuf: > + fput(memfd); > + kfree(ubuf->pages); > + kfree(ubuf); > + return ret; > +} > + > +static long udmabuf_ioctl_create(struct file *filp, unsigned long arg) > +{ > + struct udmabuf_create create; > + struct udmabuf_create_list head; > + struct udmabuf_create_item list; > + > + if (copy_from_user(&create, (void __user *)arg, > + sizeof(struct udmabuf_create))) sizeof(create) > + return -EFAULT; > + > + head.flags = create.flags; > + head.count = 1; > + list.memfd = create.memfd; > + list.offset = create.offset; > + list.size = create.size; > + > + return udmabuf_create(&head, &list); > +} > + > +static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg) > +{ > + struct udmabuf_create_list head; > + struct udmabuf_create_item *list; > + int ret = -EINVAL; > + u32 lsize; > + > + if (copy_from_user(&head, (void __user *)arg, sizeof(head))) > + return -EFAULT; > + if (head.count > 1024) > + return -EINVAL; > + lsize = sizeof(struct udmabuf_create_item) * head.count; > + list = memdup_user((void __user *)(arg + sizeof(head)), lsize); > + if (IS_ERR(list)) > + return PTR_ERR(list); > + > + ret = udmabuf_create(&head, list); > + kfree(list); > + 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; > + case UDMABUF_CREATE_LIST: > + ret = udmabuf_ioctl_create_list(filp, arg); > + break; > + default: > + ret = -EINVAL; The proper error code for invalid ioctls is -ENOTTY. > + 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@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..376b1d6730 > --- /dev/null > +++ b/tools/testing/selftests/drivers/dma-buf/udmabuf.c > @@ -0,0 +1,96 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#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 <sys/syscall.h> > +#include <linux/memfd.h> > +#include <linux/udmabuf.h> > + > +#define TEST_PREFIX "drivers/dma-buf/udmabuf" > +#define NUM_PAGES 4 > + > +static int memfd_create(const char *name, unsigned int flags) > +{ > + return syscall(__NR_memfd_create, name, flags); > +} > + > +int main(int argc, char *argv[]) > +{ > + struct udmabuf_create create; > + int devfd, memfd, buf, ret; > + off_t size; > + void *mem; > + > + devfd = open("/dev/udmabuf", O_RDWR); > + if (devfd < 0) { > + printf("%s: [skip,no-udmabuf]\n", TEST_PREFIX); > + exit(77); > + } > + > + memfd = memfd_create("udmabuf-test", MFD_CLOEXEC); > + if (memfd < 0) { > + printf("%s: [skip,no-memfd]\n", TEST_PREFIX); > + exit(77); > + } > + > + size = getpagesize() * NUM_PAGES; > + ret = ftruncate(memfd, size); > + if (ret == -1) { > + printf("%s: [FAIL,memfd-truncate]\n", TEST_PREFIX); > + exit(1); > + } > + > + memset(&create, 0, sizeof(create)); > + > + /* should fail (offset not page aligned) */ > + create.memfd = memfd; > + create.offset = getpagesize()/2; > + create.size = getpagesize(); > + buf = ioctl(devfd, UDMABUF_CREATE, &create); > + if (buf >= 0) { > + printf("%s: [FAIL,test-1]\n", TEST_PREFIX); > + exit(1); > + } > + > + /* should fail (size not multiple of page) */ > + create.memfd = memfd; > + create.offset = 0; > + create.size = getpagesize()/2; > + buf = ioctl(devfd, UDMABUF_CREATE, &create); > + if (buf >= 0) { > + printf("%s: [FAIL,test-2]\n", TEST_PREFIX); > + exit(1); > + } > + > + /* should fail (not memfd) */ > + create.memfd = 0; /* stdin */ > + create.offset = 0; > + create.size = size; > + buf = ioctl(devfd, UDMABUF_CREATE, &create); > + if (buf >= 0) { > + printf("%s: [FAIL,test-3]\n", TEST_PREFIX); > + exit(1); > + } > + > + /* should work */ > + create.memfd = memfd; > + create.offset = 0; > + create.size = size; > + buf = ioctl(devfd, UDMABUF_CREATE, &create); > + if (buf < 0) { > + printf("%s: [FAIL,test-4]\n", TEST_PREFIX); > + exit(1); > + } > + > + fprintf(stderr, "%s: ok\n", TEST_PREFIX); > + close(buf); > + close(memfd); > + close(devfd); > + return 0; > +} > diff --git a/MAINTAINERS b/MAINTAINERS > index a5b256b259..11a9b04277 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14934,6 +14934,14 @@ S: Maintained > F: Documentation/filesystems/udf.txt > F: fs/udf/ > > +UDMABUF DRIVER > +M: Gerd Hoffmann <kraxel@redhat.com> > +L: dri-devel@lists.freedesktop.org > +S: Maintained > +F: drivers/dma-buf/udmabuf.c > +F: include/uapi/linux/udmabuf.h > +F: tools/testing/selftests/drivers/dma-buf/udmabuf.c > + > UDRAW TABLET > M: Bastien Nocera <hadess@hadess.net> > L: linux-input@vger.kernel.org > @@ -15343,6 +15351,14 @@ F: arch/x86/um/ > F: fs/hostfs/ > F: fs/hppfs/ > > +USERSPACE DMA BUFFER DRIVER > +M: Gerd Hoffmann <kraxel@redhat.com> > +S: Maintained > +L: dri-devel@lists.freedesktop.org > +F: drivers/dma-buf/udmabuf.c > +F: include/uapi/linux/udmabuf.h > +T: git git://anongit.freedesktop.org/drm/drm-misc One entry should be enough. > USERSPACE I/O (UIO) > M: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > S: Maintained [snip]
Hi, > > +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create) > > Why do you start at 0x42 if you reserve the 0x40-0x4f range ? No particular strong reason, just that using 42 was less boring than starting with 0x40. > > +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct udmabuf_create_list) > > Where's the documentation ? :-) Isn't it simple enough? But, well, yes, I guess I can add some kerneldoc comments. > > +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; > > Just curious, when do you expect this to happen ? It should not. If it actually happens it would be a bug somewhere, thats why the WARN_ON. > > + struct udmabuf *ubuf; > > + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); > > sizeof(*ubuf) Why? Should not make a difference ... > > + memfd = fget(list[i].memfd); > > + if (!memfd) > > + goto err_put_pages; > > + if (!shmem_mapping(file_inode(memfd)->i_mapping)) > > + goto err_put_pages; > > + seals = memfd_fcntl(memfd, F_GET_SEALS, 0); > > + if (seals == -EINVAL || > > + (seals & SEALS_WANTED) != SEALS_WANTED || > > + (seals & SEALS_DENIED) != 0) > > + goto err_put_pages; > > All these conditions will return -EINVAL. I'm not familiar with the memfd API, > should some error conditions return a different error code to make them > distinguishable by userspace ? Hmm, I guess EBADFD would be reasonable in case the file handle isn't a memfd. Other suggestions? I'll prepare a fixup patch series addressing most of the other review comments. cheers, Gerd
Hi Gerd, On Tuesday, 11 September 2018 09:50:14 EEST Gerd Hoffmann wrote: > Hi, > > >> +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create) > > > > Why do you start at 0x42 if you reserve the 0x40-0x4f range ? > > No particular strong reason, just that using 42 was less boring than > starting with 0x40. > > >> +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct > >> udmabuf_create_list) > > > > Where's the documentation ? :-) > > Isn't it simple enough? No kernel UAPI is simple enough to get away without documenting it. > But, well, yes, I guess I can add some kerneldoc comments. > > >> +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; > > > > Just curious, when do you expect this to happen ? > > It should not. If it actually happens it would be a bug somewhere, > thats why the WARN_ON. But you seem to consider that this condition that should never happen still has a high enough chance of happening that it's worth a WARN_ON(). I was wondering why this one in particular, and not other conditions that also can't happen and are not checked through the code. > >> + struct udmabuf *ubuf; > >> > >> + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); > > > > sizeof(*ubuf) > > Why? Should not make a difference ... Because the day we replace struct udmabuf *ubuf; with struct udmabuf_ext *ubuf; and forget to change the next line, we'll introduce a bug. That's why sizeof(variable) is preferred over sizeof(type). Another reason is that I can easily see that ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL); is correct, while using sizeof(type) requires me to go and look up the declaration of the variable. > >> + memfd = fget(list[i].memfd); > >> + if (!memfd) > >> + goto err_put_pages; > >> + if (!shmem_mapping(file_inode(memfd)->i_mapping)) > >> + goto err_put_pages; > >> + seals = memfd_fcntl(memfd, F_GET_SEALS, 0); > >> + if (seals == -EINVAL || > >> + (seals & SEALS_WANTED) != SEALS_WANTED || > >> + (seals & SEALS_DENIED) != 0) > >> + goto err_put_pages; > > > > All these conditions will return -EINVAL. I'm not familiar with the memfd > > API, should some error conditions return a different error code to make > > them distinguishable by userspace ? > > Hmm, I guess EBADFD would be reasonable in case the file handle isn't a > memfd. Other suggestions? I'll let others comment on this as I don't feel qualified to pick proper error codes, not being familiar with the memfd API. > I'll prepare a fixup patch series addressing most of the other > review comments.
On Tue, Sep 11, 2018 at 11:50 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Gerd, > > On Tuesday, 11 September 2018 09:50:14 EEST Gerd Hoffmann wrote: >> Hi, >> >> >> +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create) >> > >> > Why do you start at 0x42 if you reserve the 0x40-0x4f range ? >> >> No particular strong reason, just that using 42 was less boring than >> starting with 0x40. >> >> >> +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct >> >> udmabuf_create_list) >> > >> > Where's the documentation ? :-) >> >> Isn't it simple enough? > > No kernel UAPI is simple enough to get away without documenting it. Simplest option would be to throw a bit of kerneldoc into the uapi header, add Documentation/driver-api/dma-buf.rst. -Daniel > >> But, well, yes, I guess I can add some kerneldoc comments. >> >> >> +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; >> > >> > Just curious, when do you expect this to happen ? >> >> It should not. If it actually happens it would be a bug somewhere, >> thats why the WARN_ON. > > But you seem to consider that this condition that should never happen still > has a high enough chance of happening that it's worth a WARN_ON(). I was > wondering why this one in particular, and not other conditions that also can't > happen and are not checked through the code. > >> >> + struct udmabuf *ubuf; >> >> >> >> + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); >> > >> > sizeof(*ubuf) >> >> Why? Should not make a difference ... > > Because the day we replace > > struct udmabuf *ubuf; > > with > > struct udmabuf_ext *ubuf; > > and forget to change the next line, we'll introduce a bug. That's why > sizeof(variable) is preferred over sizeof(type). Another reason is that I can > easily see that > > ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL); > > is correct, while using sizeof(type) requires me to go and look up the > declaration of the variable. > >> >> + memfd = fget(list[i].memfd); >> >> + if (!memfd) >> >> + goto err_put_pages; >> >> + if (!shmem_mapping(file_inode(memfd)->i_mapping)) >> >> + goto err_put_pages; >> >> + seals = memfd_fcntl(memfd, F_GET_SEALS, 0); >> >> + if (seals == -EINVAL || >> >> + (seals & SEALS_WANTED) != SEALS_WANTED || >> >> + (seals & SEALS_DENIED) != 0) >> >> + goto err_put_pages; >> > >> > All these conditions will return -EINVAL. I'm not familiar with the memfd >> > API, should some error conditions return a different error code to make >> > them distinguishable by userspace ? >> >> Hmm, I guess EBADFD would be reasonable in case the file handle isn't a >> memfd. Other suggestions? > > I'll let others comment on this as I don't feel qualified to pick proper error > codes, not being familiar with the memfd API. > >> I'll prepare a fixup patch series addressing most of the other >> review comments. > > -- > Regards, > > Laurent Pinchart > > >
> > >> + if (WARN_ON(vmf->pgoff >= ubuf->pagecount)) > > >> + return VM_FAULT_SIGBUS; > > > > > > Just curious, when do you expect this to happen ? > > > > It should not. If it actually happens it would be a bug somewhere, > > thats why the WARN_ON. > > But you seem to consider that this condition that should never happen still > has a high enough chance of happening that it's worth a WARN_ON(). I was > wondering why this one in particular, and not other conditions that also can't > happen and are not checked through the code. Added it while writing the code, to get any coding mistake I make flagged right away instead of things exploding later on. I can drop it. > > >> + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); > > > > > > sizeof(*ubuf) > > > > Why? Should not make a difference ... > > Because the day we replace > > struct udmabuf *ubuf; > > with > > struct udmabuf_ext *ubuf; > > and forget to change the next line, we'll introduce a bug. That's why > sizeof(variable) is preferred over sizeof(type). Another reason is that I can > easily see that > > ubuf = kzalloc(sizeof(*ubuf), GFP_KERNEL); > > is correct, while using sizeof(type) requires me to go and look up the > declaration of the variable. So it simplifies review, ok, will change it. BTW: Maybe the kernel should pick up a neat trick from glib: g_new0() is a macro which takes the type instead of the size as first argument, and it casts the return value to that type. So the compiler will throw warnings in case of a mismatch. That'll work better than depending purely on the coder being careful and review catching the remaining issues. cheers, Gerd
Hi, Le lundi 27 août 2018 à 11:34 +0200, Gerd Hoffmann a écrit : > A driver to let userspace turn memfd regions 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. > > qemu test branch: > https://git.kraxel.org/cgit/qemu/log/?h=sirius/udmabuf > > Cc: David Airlie <airlied@linux.ie> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > Documentation/ioctl/ioctl-number.txt | 1 + > include/uapi/linux/udmabuf.h | 33 +++ > drivers/dma-buf/udmabuf.c | 287 > ++++++++++++++++++++++ > tools/testing/selftests/drivers/dma-buf/udmabuf.c | 96 ++++++++ > MAINTAINERS | 16 ++ > drivers/dma-buf/Kconfig | 8 + > drivers/dma-buf/Makefile | 1 + > tools/testing/selftests/drivers/dma-buf/Makefile | 5 + > 8 files changed, 447 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..46b6532ed8 > --- /dev/null > +++ b/include/uapi/linux/udmabuf.h > @@ -0,0 +1,33 @@ > +/* 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> > + > +#define UDMABUF_FLAGS_CLOEXEC 0x01 > + > +struct udmabuf_create { > + __u32 memfd; > + __u32 flags; > + __u64 offset; > + __u64 size; > +}; > + > +struct udmabuf_create_item { > + __u32 memfd; > + __u32 __pad; > + __u64 offset; > + __u64 size; > +}; > + > +struct udmabuf_create_list { > + __u32 flags; > + __u32 count; > + struct udmabuf_create_item list[]; > +}; > + > +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create) > +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct > udmabuf_create_list) > + > +#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..8e24204526 > --- /dev/null > +++ b/drivers/dma-buf/udmabuf.c > +static long udmabuf_create(struct udmabuf_create_list *head, > + struct udmabuf_create_item *list) > +{ > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > + struct file *memfd = NULL; > + struct udmabuf *ubuf; > + struct dma_buf *buf; > + pgoff_t pgoff, pgcnt, pgidx, pgbuf; > + struct page *page; > + int seals, ret = -EINVAL; > + u32 i, flags; > + > + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); > + if (!ubuf) > + return -ENOMEM; > + > + for (i = 0; i < head->count; i++) { You need to check .__pad for unsupported value: if (list[i].__pad) { ret = -EINVAL; goto err_free_ubuf; } > + if (!IS_ALIGNED(list[i].offset, PAGE_SIZE)) > + goto err_free_ubuf; > + if (!IS_ALIGNED(list[i].size, PAGE_SIZE)) > + goto err_free_ubuf; > + ubuf->pagecount += list[i].size >> PAGE_SHIFT; > + } > + ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page > *), > + GFP_KERNEL); > + if (!ubuf->pages) { > + ret = -ENOMEM; > + goto err_free_ubuf; > + } > + > + pgbuf = 0; > + for (i = 0; i < head->count; i++) { > + memfd = fget(list[i].memfd); > + if (!memfd) > + goto err_put_pages; > + if (!shmem_mapping(file_inode(memfd)->i_mapping)) > + goto err_put_pages; > + seals = memfd_fcntl(memfd, F_GET_SEALS, 0); > + if (seals == -EINVAL || > + (seals & SEALS_WANTED) != SEALS_WANTED || > + (seals & SEALS_DENIED) != 0) > + goto err_put_pages; > + pgoff = list[i].offset >> PAGE_SHIFT; > + pgcnt = list[i].size >> PAGE_SHIFT; > + for (pgidx = 0; pgidx < pgcnt; pgidx++) { > + page = shmem_read_mapping_page( > + file_inode(memfd)->i_mapping, pgoff + > pgidx); > + if (IS_ERR(page)) { > + ret = PTR_ERR(page); > + goto err_put_pages; > + } > + ubuf->pages[pgbuf++] = page; > + } > + fput(memfd); > + } > + memfd = NULL; > + > + 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; You need to check .flags for unsupported value: if (head->flags & ~UDMABUF_FLAGS_CLOEXEC) return -EINVAL; (at the beginning of the function, of course). > + if (head->flags & UDMABUF_FLAGS_CLOEXEC) > + flags |= O_CLOEXEC; > + return dma_buf_fd(buf, flags); > + > +err_put_pages: > + while (pgbuf > 0) > + put_page(ubuf->pages[--pgbuf]); > +err_free_ubuf: > + fput(memfd); > + kfree(ubuf->pages); > + kfree(ubuf); > + return ret; > +} > + Regards
On Wed, Sep 12, 2018 at 12:03 AM Yann Droneaud <ydroneaud@opteya.com> wrote: > > Hi, > > Le lundi 27 août 2018 à 11:34 +0200, Gerd Hoffmann a écrit : > > A driver to let userspace turn memfd regions 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. Something like this is definitely needed. I assume one flow will be: 1) guest compositor allocates a buffer using udmabuf 2) 3D driver imports the udmabuf and renders to it 3) qemu turns this udmabuf into a host dma-buf 4) host compositor displays this dma-buf In that case, how does the guest know about the host's stride / alignment restrictions? For example, x-tiling on Intel (good for display) needs to have a stride that's a multiple of 512 bytes. > > > > qemu test branch: > > https://git.kraxel.org/cgit/qemu/log/?h=sirius/udmabuf > > > > Cc: David Airlie <airlied@linux.ie> > > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > Documentation/ioctl/ioctl-number.txt | 1 + > > include/uapi/linux/udmabuf.h | 33 +++ > > drivers/dma-buf/udmabuf.c | 287 > > ++++++++++++++++++++++ > > tools/testing/selftests/drivers/dma-buf/udmabuf.c | 96 ++++++++ > > MAINTAINERS | 16 ++ > > drivers/dma-buf/Kconfig | 8 + > > drivers/dma-buf/Makefile | 1 + > > tools/testing/selftests/drivers/dma-buf/Makefile | 5 + > > 8 files changed, 447 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..46b6532ed8 > > --- /dev/null > > +++ b/include/uapi/linux/udmabuf.h > > @@ -0,0 +1,33 @@ > > +/* 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> > > + > > +#define UDMABUF_FLAGS_CLOEXEC 0x01 > > + > > +struct udmabuf_create { > > + __u32 memfd; > > + __u32 flags; > > + __u64 offset; > > + __u64 size; > > +}; > > + > > +struct udmabuf_create_item { > > + __u32 memfd; > > + __u32 __pad; > > + __u64 offset; > > + __u64 size; > > +}; > > + > > +struct udmabuf_create_list { > > + __u32 flags; > > + __u32 count; > > + struct udmabuf_create_item list[]; > > +}; > > + > > +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create) > > +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct > > udmabuf_create_list) > > + > > +#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..8e24204526 > > --- /dev/null > > +++ b/drivers/dma-buf/udmabuf.c > > +static long udmabuf_create(struct udmabuf_create_list *head, > > + struct udmabuf_create_item *list) > > +{ > > + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); > > + struct file *memfd = NULL; > > + struct udmabuf *ubuf; > > + struct dma_buf *buf; > > + pgoff_t pgoff, pgcnt, pgidx, pgbuf; > > + struct page *page; > > + int seals, ret = -EINVAL; > > + u32 i, flags; > > + > > + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); > > + if (!ubuf) > > + return -ENOMEM; > > + > > + for (i = 0; i < head->count; i++) { > > You need to check .__pad for unsupported value: > > if (list[i].__pad) { > ret = -EINVAL; > goto err_free_ubuf; > } > > > + if (!IS_ALIGNED(list[i].offset, PAGE_SIZE)) > > + goto err_free_ubuf; > > + if (!IS_ALIGNED(list[i].size, PAGE_SIZE)) > > + goto err_free_ubuf; > > + ubuf->pagecount += list[i].size >> PAGE_SHIFT; > > + } > > + ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page > > *), > > + GFP_KERNEL); > > + if (!ubuf->pages) { > > + ret = -ENOMEM; > > + goto err_free_ubuf; > > + } > > + > > + pgbuf = 0; > > + for (i = 0; i < head->count; i++) { > > + memfd = fget(list[i].memfd); > > + if (!memfd) > > + goto err_put_pages; > > + if (!shmem_mapping(file_inode(memfd)->i_mapping)) > > + goto err_put_pages; > > + seals = memfd_fcntl(memfd, F_GET_SEALS, 0); > > + if (seals == -EINVAL || > > + (seals & SEALS_WANTED) != SEALS_WANTED || > > + (seals & SEALS_DENIED) != 0) > > + goto err_put_pages; > > + pgoff = list[i].offset >> PAGE_SHIFT; > > + pgcnt = list[i].size >> PAGE_SHIFT; > > + for (pgidx = 0; pgidx < pgcnt; pgidx++) { > > + page = shmem_read_mapping_page( > > + file_inode(memfd)->i_mapping, pgoff + > > pgidx); > > + if (IS_ERR(page)) { > > + ret = PTR_ERR(page); > > + goto err_put_pages; > > + } > > + ubuf->pages[pgbuf++] = page; > > + } > > + fput(memfd); > > + } > > + memfd = NULL; > > + > > + 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; > > You need to check .flags for unsupported value: > > if (head->flags & ~UDMABUF_FLAGS_CLOEXEC) > return -EINVAL; > > (at the beginning of the function, of course). > > > + if (head->flags & UDMABUF_FLAGS_CLOEXEC) > > + flags |= O_CLOEXEC; > > + return dma_buf_fd(buf, flags); > > + > > +err_put_pages: > > + while (pgbuf > 0) > > + put_page(ubuf->pages[--pgbuf]); > > +err_free_ubuf: > > + fput(memfd); > > + kfree(ubuf->pages); > > + kfree(ubuf); > > + return ret; > > +} > > + > > Regards > > -- > Yann Droneaud > OPTEYA > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 12, 2018 at 08:24:00PM -0700, Gurchetan Singh wrote: > On Wed, Sep 12, 2018 at 12:03 AM Yann Droneaud <ydroneaud@opteya.com> wrote: > > > > Hi, > > > > Le lundi 27 août 2018 à 11:34 +0200, Gerd Hoffmann a écrit : > > > A driver to let userspace turn memfd regions 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. > > Something like this is definitely needed. I assume one flow will be: > > 1) guest compositor allocates a buffer using udmabuf > 2) 3D driver imports the udmabuf and renders to it > 3) qemu turns this udmabuf into a host dma-buf > 4) host compositor displays this dma-buf Well, no. This is *not* about 3D, it's about software rendering, for example cairo doing its work for gtk apps. So the workflow would be along these lines: (1) guest app allocates dumb drm buffer from virtio-gpu, renders to it. (2) guest app passes the buffer to wayland guest proxy (which looks like a wayland server/compositor to the app, but it doesn't actually composite anything). (3) wayland guest proxy passes buffer handle to wayland host proxy. (4) qemu can then use the buffer handle to lookup the virtio-gpu buffer, then use udmabuf to create a host dma-buf for it. (5) host dma-buf can be passed to host wayland server for display, so guest app window shows up seamlessly on the host. Details of the wayland protocol proxying are not hashed out yet. > In that case, how does the guest know about the host's stride / > alignment restrictions? For example, x-tiling on Intel (good for > display) needs to have a stride that's a multiple of 512 bytes. For 3D rendering (aka virgl) the workflow is quite different. The guest submits the rendering commands to the host, so the actual rendering happens on the host gpu, to a host-allocated drm buffer. Which can be exported as dma-buf by the gpu driver just fine. The guest passes resources needed for rendering (textures, ...) to the host. I'm not sure how useful udmabuf is for that, exactly because of the gpu specific formats. It's a tradeoff: On the plus side we can avoid allocating a host resource and copying the data, by creating and importing a dma-buf instead. On the other hand the host can convert the data while copying it over from the guest to a host drm buffer, to a format preferred by the host gpu (tiling, compressing, ...), so the gpu will perform better that way. cheers, Gerd
On Wed, Sep 12, 2018 at 11:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Wed, Sep 12, 2018 at 08:24:00PM -0700, Gurchetan Singh wrote: > > On Wed, Sep 12, 2018 at 12:03 AM Yann Droneaud <ydroneaud@opteya.com> wrote: > > > > > > Hi, > > > > > > Le lundi 27 août 2018 à 11:34 +0200, Gerd Hoffmann a écrit : > > > > A driver to let userspace turn memfd regions 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. > > > > Something like this is definitely needed. I assume one flow will be: > > > > 1) guest compositor allocates a buffer using udmabuf > > 2) 3D driver imports the udmabuf and renders to it > > 3) qemu turns this udmabuf into a host dma-buf > > 4) host compositor displays this dma-buf > > Well, no. This is *not* about 3D, it's about software rendering, for > example cairo doing its work for gtk apps. So the workflow would be > along these lines: > > (1) guest app allocates dumb drm buffer from virtio-gpu, renders to it. > (2) guest app passes the buffer to wayland guest proxy (which looks > like a wayland server/compositor to the app, but it doesn't actually > composite anything). > (3) wayland guest proxy passes buffer handle to wayland host proxy. > (4) qemu can then use the buffer handle to lookup the virtio-gpu > buffer, then use udmabuf to create a host dma-buf for it. > (5) host dma-buf can be passed to host wayland server for display, so > guest app window shows up seamlessly on the host. > > Details of the wayland protocol proxying are not hashed out yet. Thanks for the clarification. With dumb buffers, I guess the host can use DRM_FORMAT_MOD_LINEAR when sending the udmabuf-created buffer to the display. Note there are certain cases where tiled buffers are map-able (Intel), and with some variation of virtio_gpu_resource_create_coherent, we could expose that to the guest. That's the direction we're interested in going, though it looks like udmabuf is orthogonal to that. What details on wayland protocol proxying still need to be worked out? That's one component of the ChromiumOS solution (virtio_wl) that hasn't been up-streamed yet. That method maps guest fds to host fds. > > In that case, how does the guest know about the host's stride / > > alignment restrictions? For example, x-tiling on Intel (good for > > display) needs to have a stride that's a multiple of 512 bytes. > > For 3D rendering (aka virgl) the workflow is quite different. The guest > submits the rendering commands to the host, so the actual rendering > happens on the host gpu, to a host-allocated drm buffer. Which can be > exported as dma-buf by the gpu driver just fine. > > The guest passes resources needed for rendering (textures, ...) to the > host. I'm not sure how useful udmabuf is for that, exactly because of > the gpu specific formats. It's a tradeoff: On the plus side we can > avoid allocating a host resource and copying the data, by creating and > importing a dma-buf instead. On the other hand the host can convert the > data while copying it over from the guest to a host drm buffer, to a > format preferred by the host gpu (tiling, compressing, ...), so the gpu > will perform better that way. > > cheers, > Gerd >
Hi, > > Well, no. This is *not* about 3D, it's about software rendering, for > > example cairo doing its work for gtk apps. So the workflow would be > > along these lines: > > > > (1) guest app allocates dumb drm buffer from virtio-gpu, renders to it. > > (2) guest app passes the buffer to wayland guest proxy (which looks > > like a wayland server/compositor to the app, but it doesn't actually > > composite anything). > > (3) wayland guest proxy passes buffer handle to wayland host proxy. > > (4) qemu can then use the buffer handle to lookup the virtio-gpu > > buffer, then use udmabuf to create a host dma-buf for it. > > (5) host dma-buf can be passed to host wayland server for display, so > > guest app window shows up seamlessly on the host. > > > > Details of the wayland protocol proxying are not hashed out yet. > > Thanks for the clarification. With dumb buffers, I guess the host can > use DRM_FORMAT_MOD_LINEAR when sending the udmabuf-created buffer to > the display. Note there are certain cases where tiled buffers are > map-able (Intel), and with some variation of > virtio_gpu_resource_create_coherent, we could expose that to the > guest. The coherent mapping (host-allocated resources into guest address space) is another thing which needs to be sorted out. > That's the direction we're interested in going, though it > looks like udmabuf is orthogonal to that. Yes, udmabuf is the other way around, guest allocates resources and we make them available as host dmabufs. > What details on wayland protocol proxying still need to be worked out? > That's one component of the ChromiumOS solution (virtio_wl) that > hasn't been up-streamed yet. That method maps guest fds to host fds. Tomeu Vizoso (Cc'ed) was looking into using virtio-serial as wayland protocol transport between host and guest. With udmabuf we can use virtio-gpu drm objects to pass pixel buffers from guest to host, which is one important building block for that. Not sure what the current state is. cheers, Gerd
On 09/14/2018 08:37 AM, Gerd Hoffmann wrote: > Hi, > >>> Well, no. This is *not* about 3D, it's about software rendering, for >>> example cairo doing its work for gtk apps. So the workflow would be >>> along these lines: >>> >>> (1) guest app allocates dumb drm buffer from virtio-gpu, renders to it. Why not let clients keep allocating memory for buffers with memfd? The guest proxy would then create a virtio-gpu resource to wrap the shm pool memory. The VMM would receive a number of physical addresses that can be used to create a dmabuf. The would place the new FD in the wayland protocol stream. >>> (2) guest app passes the buffer to wayland guest proxy (which looks >>> like a wayland server/compositor to the app, but it doesn't actually >>> composite anything). >>> (3) wayland guest proxy passes buffer handle to wayland host proxy. >>> (4) qemu can then use the buffer handle to lookup the virtio-gpu >>> buffer, then use udmabuf to create a host dma-buf for it. >>> (5) host dma-buf can be passed to host wayland server for display, so >>> guest app window shows up seamlessly on the host. >>> >>> Details of the wayland protocol proxying are not hashed out yet. >> >> Thanks for the clarification. With dumb buffers, I guess the host can >> use DRM_FORMAT_MOD_LINEAR when sending the udmabuf-created buffer to >> the display. Note there are certain cases where tiled buffers are >> map-able (Intel), and with some variation of >> virtio_gpu_resource_create_coherent, we could expose that to the >> guest. > > The coherent mapping (host-allocated resources into guest address space) > is another thing which needs to be sorted out. > >> That's the direction we're interested in going, though it >> looks like udmabuf is orthogonal to that. > > Yes, udmabuf is the other way around, guest allocates resources and we > make them available as host dmabufs. > >> What details on wayland protocol proxying still need to be worked out? >> That's one component of the ChromiumOS solution (virtio_wl) that >> hasn't been up-streamed yet. That method maps guest fds to host fds. > > Tomeu Vizoso (Cc'ed) was looking into using virtio-serial as wayland > protocol transport between host and guest. With udmabuf we can use > virtio-gpu drm objects to pass pixel buffers from guest to host, which > is one important building block for that. The approach that was agreed by everybody was to add ioctls to virtio-gpu to send and receive protocol messages. The guest proxy would use those ioctls and the VMM would play as the host proxy and would use similar virtio commands. There's code for that already and someone else from Collabora is to retake the upstreaming effort at some point soon. Regards, Tomeu
On Fri, Sep 14, 2018 at 02:00:30PM +0200, Tomeu Vizoso wrote: > On 09/14/2018 08:37 AM, Gerd Hoffmann wrote: > > Hi, > > > > > > Well, no. This is *not* about 3D, it's about software rendering, for > > > > example cairo doing its work for gtk apps. So the workflow would be > > > > along these lines: > > > > > > > > (1) guest app allocates dumb drm buffer from virtio-gpu, renders to it. > > Why not let clients keep allocating memory for buffers with memfd? > > The guest proxy would then create a virtio-gpu resource to wrap the shm pool > memory. Should be workable too, with udmabuf in the guest. Allocate with memfd, turn into dmabuf using udmabuf, let the virtio-gpu guest driver import the thing so you have a virtio-gpu resource handle for it. I don't see why this is better than using virtio-gpu dumb buffers directly though. cheers, Gerd
On 09/14/2018 03:00 PM, Gerd Hoffmann wrote: > On Fri, Sep 14, 2018 at 02:00:30PM +0200, Tomeu Vizoso wrote: >> On 09/14/2018 08:37 AM, Gerd Hoffmann wrote: >>> Hi, >>> >>>>> Well, no. This is *not* about 3D, it's about software rendering, for >>>>> example cairo doing its work for gtk apps. So the workflow would be >>>>> along these lines: >>>>> >>>>> (1) guest app allocates dumb drm buffer from virtio-gpu, renders to it. >> >> Why not let clients keep allocating memory for buffers with memfd? >> >> The guest proxy would then create a virtio-gpu resource to wrap the shm pool >> memory. > > Should be workable too, with udmabuf in the guest. Allocate with memfd, > turn into dmabuf using udmabuf, let the virtio-gpu guest driver import > the thing so you have a virtio-gpu resource handle for it. > > I don't see why this is better than using virtio-gpu dumb buffers > directly though. Because wl_shm clients are allocating memory with memfd already, so they would need to be modified so they allocate dumb buffers instead. In practice, that means only modifying gtk, qt and sdl. Cheers, Tomeu
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt index 13a7c999c0..f2ac672eb7 100644 --- a/Documentation/ioctl/ioctl-number.txt +++ b/Documentation/ioctl/ioctl-number.txt @@ -272,6 +272,7 @@ Code Seq#(hex) Include File Comments 't' 90-91 linux/toshiba.h toshiba and toshiba_acpi SMM 'u' 00-1F linux/smb_fs.h gone 'u' 20-3F linux/uvcvideo.h USB video class host driver +'u' 40-4f linux/udmabuf.h userspace dma-buf misc device 'v' 00-1F linux/ext2_fs.h conflict! 'v' 00-1F linux/fs.h conflict! 'v' 00-0F linux/sonypi.h conflict! diff --git a/include/uapi/linux/udmabuf.h b/include/uapi/linux/udmabuf.h new file mode 100644 index 0000000000..46b6532ed8 --- /dev/null +++ b/include/uapi/linux/udmabuf.h @@ -0,0 +1,33 @@ +/* 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> + +#define UDMABUF_FLAGS_CLOEXEC 0x01 + +struct udmabuf_create { + __u32 memfd; + __u32 flags; + __u64 offset; + __u64 size; +}; + +struct udmabuf_create_item { + __u32 memfd; + __u32 __pad; + __u64 offset; + __u64 size; +}; + +struct udmabuf_create_list { + __u32 flags; + __u32 count; + struct udmabuf_create_item list[]; +}; + +#define UDMABUF_CREATE _IOW('u', 0x42, struct udmabuf_create) +#define UDMABUF_CREATE_LIST _IOW('u', 0x43, struct udmabuf_create_list) + +#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..8e24204526 --- /dev/null +++ b/drivers/dma-buf/udmabuf.c @@ -0,0 +1,287 @@ +// SPDX-License-Identifier: GPL-2.0 +#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 <linux/shmem_fs.h> +#include <linux/memfd.h> + +#include <uapi/linux/udmabuf.h> + +struct udmabuf { + u32 pagecount; + struct page **pages; +}; + +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 | VM_MAYSHARE)) == 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]); + kfree(ubuf->pages); + kfree(ubuf); +} + +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 void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num, + void *vaddr) +{ + kunmap(vaddr); +} + +static struct dma_buf_ops udmabuf_ops = { + .map_dma_buf = map_udmabuf, + .unmap_dma_buf = unmap_udmabuf, + .release = release_udmabuf, + .map = kmap_udmabuf, + .unmap = kunmap_udmabuf, + .mmap = mmap_udmabuf, +}; + +#define SEALS_WANTED (F_SEAL_SHRINK) +#define SEALS_DENIED (F_SEAL_WRITE) + +static long udmabuf_create(struct udmabuf_create_list *head, + struct udmabuf_create_item *list) +{ + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); + struct file *memfd = NULL; + struct udmabuf *ubuf; + struct dma_buf *buf; + pgoff_t pgoff, pgcnt, pgidx, pgbuf; + struct page *page; + int seals, ret = -EINVAL; + u32 i, flags; + + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); + if (!ubuf) + return -ENOMEM; + + for (i = 0; i < head->count; i++) { + if (!IS_ALIGNED(list[i].offset, PAGE_SIZE)) + goto err_free_ubuf; + if (!IS_ALIGNED(list[i].size, PAGE_SIZE)) + goto err_free_ubuf; + ubuf->pagecount += list[i].size >> PAGE_SHIFT; + } + ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page *), + GFP_KERNEL); + if (!ubuf->pages) { + ret = -ENOMEM; + goto err_free_ubuf; + } + + pgbuf = 0; + for (i = 0; i < head->count; i++) { + memfd = fget(list[i].memfd); + if (!memfd) + goto err_put_pages; + if (!shmem_mapping(file_inode(memfd)->i_mapping)) + goto err_put_pages; + seals = memfd_fcntl(memfd, F_GET_SEALS, 0); + if (seals == -EINVAL || + (seals & SEALS_WANTED) != SEALS_WANTED || + (seals & SEALS_DENIED) != 0) + goto err_put_pages; + pgoff = list[i].offset >> PAGE_SHIFT; + pgcnt = list[i].size >> PAGE_SHIFT; + for (pgidx = 0; pgidx < pgcnt; pgidx++) { + page = shmem_read_mapping_page( + file_inode(memfd)->i_mapping, pgoff + pgidx); + if (IS_ERR(page)) { + ret = PTR_ERR(page); + goto err_put_pages; + } + ubuf->pages[pgbuf++] = page; + } + fput(memfd); + } + memfd = NULL; + + 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 (head->flags & UDMABUF_FLAGS_CLOEXEC) + flags |= O_CLOEXEC; + return dma_buf_fd(buf, flags); + +err_put_pages: + while (pgbuf > 0) + put_page(ubuf->pages[--pgbuf]); +err_free_ubuf: + fput(memfd); + kfree(ubuf->pages); + kfree(ubuf); + return ret; +} + +static long udmabuf_ioctl_create(struct file *filp, unsigned long arg) +{ + struct udmabuf_create create; + struct udmabuf_create_list head; + struct udmabuf_create_item list; + + if (copy_from_user(&create, (void __user *)arg, + sizeof(struct udmabuf_create))) + return -EFAULT; + + head.flags = create.flags; + head.count = 1; + list.memfd = create.memfd; + list.offset = create.offset; + list.size = create.size; + + return udmabuf_create(&head, &list); +} + +static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg) +{ + struct udmabuf_create_list head; + struct udmabuf_create_item *list; + int ret = -EINVAL; + u32 lsize; + + if (copy_from_user(&head, (void __user *)arg, sizeof(head))) + return -EFAULT; + if (head.count > 1024) + return -EINVAL; + lsize = sizeof(struct udmabuf_create_item) * head.count; + list = memdup_user((void __user *)(arg + sizeof(head)), lsize); + if (IS_ERR(list)) + return PTR_ERR(list); + + ret = udmabuf_create(&head, list); + kfree(list); + 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; + case UDMABUF_CREATE_LIST: + ret = udmabuf_ioctl_create_list(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@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..376b1d6730 --- /dev/null +++ b/tools/testing/selftests/drivers/dma-buf/udmabuf.c @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: GPL-2.0 +#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 <sys/syscall.h> +#include <linux/memfd.h> +#include <linux/udmabuf.h> + +#define TEST_PREFIX "drivers/dma-buf/udmabuf" +#define NUM_PAGES 4 + +static int memfd_create(const char *name, unsigned int flags) +{ + return syscall(__NR_memfd_create, name, flags); +} + +int main(int argc, char *argv[]) +{ + struct udmabuf_create create; + int devfd, memfd, buf, ret; + off_t size; + void *mem; + + devfd = open("/dev/udmabuf", O_RDWR); + if (devfd < 0) { + printf("%s: [skip,no-udmabuf]\n", TEST_PREFIX); + exit(77); + } + + memfd = memfd_create("udmabuf-test", MFD_CLOEXEC); + if (memfd < 0) { + printf("%s: [skip,no-memfd]\n", TEST_PREFIX); + exit(77); + } + + size = getpagesize() * NUM_PAGES; + ret = ftruncate(memfd, size); + if (ret == -1) { + printf("%s: [FAIL,memfd-truncate]\n", TEST_PREFIX); + exit(1); + } + + memset(&create, 0, sizeof(create)); + + /* should fail (offset not page aligned) */ + create.memfd = memfd; + create.offset = getpagesize()/2; + create.size = getpagesize(); + buf = ioctl(devfd, UDMABUF_CREATE, &create); + if (buf >= 0) { + printf("%s: [FAIL,test-1]\n", TEST_PREFIX); + exit(1); + } + + /* should fail (size not multiple of page) */ + create.memfd = memfd; + create.offset = 0; + create.size = getpagesize()/2; + buf = ioctl(devfd, UDMABUF_CREATE, &create); + if (buf >= 0) { + printf("%s: [FAIL,test-2]\n", TEST_PREFIX); + exit(1); + } + + /* should fail (not memfd) */ + create.memfd = 0; /* stdin */ + create.offset = 0; + create.size = size; + buf = ioctl(devfd, UDMABUF_CREATE, &create); + if (buf >= 0) { + printf("%s: [FAIL,test-3]\n", TEST_PREFIX); + exit(1); + } + + /* should work */ + create.memfd = memfd; + create.offset = 0; + create.size = size; + buf = ioctl(devfd, UDMABUF_CREATE, &create); + if (buf < 0) { + printf("%s: [FAIL,test-4]\n", TEST_PREFIX); + exit(1); + } + + fprintf(stderr, "%s: ok\n", TEST_PREFIX); + close(buf); + close(memfd); + close(devfd); + return 0; +} diff --git a/MAINTAINERS b/MAINTAINERS index a5b256b259..11a9b04277 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14934,6 +14934,14 @@ S: Maintained F: Documentation/filesystems/udf.txt F: fs/udf/ +UDMABUF DRIVER +M: Gerd Hoffmann <kraxel@redhat.com> +L: dri-devel@lists.freedesktop.org +S: Maintained +F: drivers/dma-buf/udmabuf.c +F: include/uapi/linux/udmabuf.h +F: tools/testing/selftests/drivers/dma-buf/udmabuf.c + UDRAW TABLET M: Bastien Nocera <hadess@hadess.net> L: linux-input@vger.kernel.org @@ -15343,6 +15351,14 @@ F: arch/x86/um/ F: fs/hostfs/ F: fs/hppfs/ +USERSPACE DMA BUFFER DRIVER +M: Gerd Hoffmann <kraxel@redhat.com> +S: Maintained +L: dri-devel@lists.freedesktop.org +F: drivers/dma-buf/udmabuf.c +F: include/uapi/linux/udmabuf.h +T: git git://anongit.freedesktop.org/drm/drm-misc + USERSPACE I/O (UIO) M: Greg Kroah-Hartman <gregkh@linuxfoundation.org> S: Maintained diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index ed3b785bae..338129eb12 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -30,4 +30,12 @@ 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 memfd regions into dma-bufs. + Qemu can use this to create host dmabufs for guest framebuffers. + 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
A driver to let userspace turn memfd regions 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. qemu test branch: https://git.kraxel.org/cgit/qemu/log/?h=sirius/udmabuf Cc: David Airlie <airlied@linux.ie> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Daniel Vetter <daniel@ffwll.ch> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- Documentation/ioctl/ioctl-number.txt | 1 + include/uapi/linux/udmabuf.h | 33 +++ drivers/dma-buf/udmabuf.c | 287 ++++++++++++++++++++++ tools/testing/selftests/drivers/dma-buf/udmabuf.c | 96 ++++++++ MAINTAINERS | 16 ++ drivers/dma-buf/Kconfig | 8 + drivers/dma-buf/Makefile | 1 + tools/testing/selftests/drivers/dma-buf/Makefile | 5 + 8 files changed, 447 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