[RFC 0/6] udmabuf guest <--> host interaction model

[RFC 0/6] udmabuf guest <--> host interaction model

[Gurchetan Singh]

It's desirable to use zero-copy mechanisms when using various graphics
buffers. With udmabuf, two cases come to mind:

1) Directly scan-out a guest-mapped buffer
2) Import into VK with VK_EXT_external_memory_dma_buf

However, displays are not generally coherent with the CPU and many
GPUs aren't either.  So we need to map write-combine in the guest.
One way to achieve this is to synchronize on the host.

[warning] -- only compile tested at this point to get feedback

Gurchetan Singh (6):
  udmabuf: use cache_sgt_mapping option
  udmabuf: add ability to set access flags on udmabuf
  udmabuf: enforce access flags
  udmabuf: add a pointer to the miscdevice in dma-buf private data
  udmabuf: separate out creating/destroying scatter-table
  udmabuf: implement begin_cpu_access/end_cpu_access hooks

 drivers/dma-buf/udmabuf.c    | 103 +++++++++++++++++++++++++++++------
 include/uapi/linux/udmabuf.h |   5 +-
 2 files changed, 90 insertions(+), 18 deletions(-)

-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 1/6] udmabuf: use cache_sgt_mapping option

[Gurchetan Singh]

The GEM prime helpers do it, so should we. It's also possible to make
it optional later.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/dma-buf/udmabuf.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 9635897458a0..b345e91d831a 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -108,12 +108,13 @@ static void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num,
 }
 
 static const 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,
+	.cache_sgt_mapping = true,
+	.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)
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 2/6] udmabuf: add ability to set access flags on udmabuf

[Gurchetan Singh]

The main use for udmabuf is sending guest memory pages
to the host.

It's generally a bad idea to have to separate mappings with
different attributes. For example, a WC mapping the guest
kernel and cached mapping on the host is problematic.

Add creation time flags so the user has responsibility for
the specific use case.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/dma-buf/udmabuf.c    | 7 ++++++-
 include/uapi/linux/udmabuf.h | 5 ++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index b345e91d831a..4ecf2a94fed3 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -186,7 +186,12 @@ static long udmabuf_create(const struct udmabuf_create_list *head,
 	exp_info.ops  = &udmabuf_ops;
 	exp_info.size = ubuf->pagecount << PAGE_SHIFT;
 	exp_info.priv = ubuf;
-	exp_info.flags = O_RDWR;
+
+	if ((head->flags & UDMABUF_FLAGS_PROT_READ) &&
+	    (head->flags & UDMABUF_FLAGS_PROT_WRITE))
+		exp_info.flags = O_RDWR;
+	else if (head->flags & UDMABUF_FLAGS_PROT_WRITE)
+		exp_info.flags = O_WRONLY;
 
 	buf = dma_buf_export(&exp_info);
 	if (IS_ERR(buf)) {
diff --git a/include/uapi/linux/udmabuf.h b/include/uapi/linux/udmabuf.h
index 46b6532ed855..21290cb9696c 100644
--- a/include/uapi/linux/udmabuf.h
+++ b/include/uapi/linux/udmabuf.h
@@ -5,7 +5,10 @@
 #include <linux/types.h>
 #include <linux/ioctl.h>
 
-#define UDMABUF_FLAGS_CLOEXEC	0x01
+#define UDMABUF_FLAGS_CLOEXEC    0x01
+#define UDMABUF_FLAGS_PROT_NONE  0x02
+#define UDMABUF_FLAGS_PROT_READ  0x04
+#define UDMABUF_FLAGS_PROT_WRITE 0x08
 
 struct udmabuf_create {
 	__u32 memfd;
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 3/6] udmabuf: enforce access flags

[Gurchetan Singh]

Enforce the access flags that were added earlier.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/dma-buf/udmabuf.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 4ecf2a94fed3..134e53d24c2b 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -16,6 +16,7 @@ static const u32    list_limit = 1024;  /* udmabuf_create_list->count limit */
 static const size_t size_limit_mb = 64; /* total dmabuf size, in megabytes  */
 
 struct udmabuf {
+	u32 flags;
 	pgoff_t pagecount;
 	struct page **pages;
 };
@@ -37,10 +38,17 @@ static const struct vm_operations_struct udmabuf_vm_ops = {
 static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
 {
 	struct udmabuf *ubuf = buf->priv;
+	pgprot_t pgprot = vm_get_page_prot(vma->vm_flags);
 
 	if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
 		return -EINVAL;
 
+	if (ubuf->flags & UDMABUF_FLAGS_PROT_NONE)
+		return -EINVAL;
+
+	if ((ubuf->flags & UDMABUF_FLAGS_PROT_READ) == 0)
+		vma->vm_page_prot = pgprot_writecombine(pgprot);
+
 	vma->vm_ops = &udmabuf_vm_ops;
 	vma->vm_private_data = ubuf;
 	return 0;
@@ -193,6 +201,7 @@ static long udmabuf_create(const struct udmabuf_create_list *head,
 	else if (head->flags & UDMABUF_FLAGS_PROT_WRITE)
 		exp_info.flags = O_WRONLY;
 
+	ubuf->flags = head->flags;
 	buf = dma_buf_export(&exp_info);
 	if (IS_ERR(buf)) {
 		ret = PTR_ERR(buf);
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 4/6] udmabuf: add a pointer to the miscdevice in dma-buf private data

[Gurchetan Singh]

Will be used later.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/dma-buf/udmabuf.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 134e53d24c2b..47003abbf4c2 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -19,6 +19,7 @@ struct udmabuf {
 	u32 flags;
 	pgoff_t pagecount;
 	struct page **pages;
+	struct miscdevice *udmabuf_misc;
 };
 
 static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf)
@@ -128,7 +129,8 @@ static const struct dma_buf_ops udmabuf_ops = {
 #define SEALS_WANTED (F_SEAL_SHRINK)
 #define SEALS_DENIED (F_SEAL_WRITE)
 
-static long udmabuf_create(const struct udmabuf_create_list *head,
+static long udmabuf_create(struct miscdevice *udmabuf_misc,
+			   struct udmabuf_create_list *head,
 			   const struct udmabuf_create_item *list)
 {
 	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
@@ -202,6 +204,7 @@ static long udmabuf_create(const struct udmabuf_create_list *head,
 		exp_info.flags = O_WRONLY;
 
 	ubuf->flags = head->flags;
+	ubuf->udmabuf_misc = udmabuf_misc;
 	buf = dma_buf_export(&exp_info);
 	if (IS_ERR(buf)) {
 		ret = PTR_ERR(buf);
@@ -239,7 +242,7 @@ static long udmabuf_ioctl_create(struct file *filp, unsigned long arg)
 	list.offset = create.offset;
 	list.size   = create.size;
 
-	return udmabuf_create(&head, &list);
+	return udmabuf_create(filp->private_data, &head, &list);
 }
 
 static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg)
@@ -258,7 +261,7 @@ static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg)
 	if (IS_ERR(list))
 		return PTR_ERR(list);
 
-	ret = udmabuf_create(&head, list);
+	ret = udmabuf_create(filp->private_data, &head, list);
 	kfree(list);
 	return ret;
 }
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 5/6] udmabuf: separate out creating/destroying scatter-table

[Gurchetan Singh]

Reused later.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/dma-buf/udmabuf.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 47003abbf4c2..5f8bee1862de 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -55,10 +55,10 @@ static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
 	return 0;
 }
 
-static struct sg_table *map_udmabuf(struct dma_buf_attachment *at,
-				    enum dma_data_direction direction)
+static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf,
+				     enum dma_data_direction direction)
 {
-	struct udmabuf *ubuf = at->dmabuf->priv;
+	struct udmabuf *ubuf = buf->priv;
 	struct sg_table *sg;
 	int ret;
 
@@ -70,7 +70,7 @@ static struct sg_table *map_udmabuf(struct dma_buf_attachment *at,
 					GFP_KERNEL);
 	if (ret < 0)
 		goto err;
-	if (!dma_map_sg(at->dev, sg->sgl, sg->nents, direction)) {
+	if (!dma_map_sg(dev, sg->sgl, sg->nents, direction)) {
 		ret = -EINVAL;
 		goto err;
 	}
@@ -82,13 +82,25 @@ static struct sg_table *map_udmabuf(struct dma_buf_attachment *at,
 	return ERR_PTR(ret);
 }
 
+static void put_sg_table(struct device *dev, struct sg_table *sg,
+			 enum dma_data_direction direction)
+{
+	dma_unmap_sg(dev, sg->sgl, sg->nents, direction);
+	sg_free_table(sg);
+	kfree(sg);
+}
+
+static struct sg_table *map_udmabuf(struct dma_buf_attachment *at,
+				    enum dma_data_direction direction)
+{
+	return get_sg_table(at->dev, at->dmabuf, direction);
+}
+
 static void unmap_udmabuf(struct dma_buf_attachment *at,
 			  struct sg_table *sg,
 			  enum dma_data_direction direction)
 {
-	dma_unmap_sg(at->dev, sg->sgl, sg->nents, direction);
-	sg_free_table(sg);
-	kfree(sg);
+	return put_sg_table(at->dev, sg, direction);
 }
 
 static void release_udmabuf(struct dma_buf *buf)
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 6/6] udmabuf: implement begin_cpu_access/end_cpu_access hooks

[Gurchetan Singh]

With the misc device, we should end up using dma direct ops.
This can allow us to have WC mappings in the guest after some
synchronization, if we disallow cached mappings in the host.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/dma-buf/udmabuf.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 5f8bee1862de..52de7ba1e712 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -19,6 +19,7 @@ struct udmabuf {
 	u32 flags;
 	pgoff_t pagecount;
 	struct page **pages;
+	struct sg_table *sg;
 	struct miscdevice *udmabuf_misc;
 };
 
@@ -106,8 +107,12 @@ static void unmap_udmabuf(struct dma_buf_attachment *at,
 static void release_udmabuf(struct dma_buf *buf)
 {
 	struct udmabuf *ubuf = buf->priv;
+	struct device *dev = ubuf->udmabuf_misc->this_device;
 	pgoff_t pg;
 
+	if (ubuf->sg)
+		put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL);
+
 	for (pg = 0; pg < ubuf->pagecount; pg++)
 		put_page(ubuf->pages[pg]);
 	kfree(ubuf->pages);
@@ -128,6 +133,38 @@ static void kunmap_udmabuf(struct dma_buf *buf, unsigned long page_num,
 	kunmap(vaddr);
 }
 
+static int begin_cpu_udmabuf(struct dma_buf *buf,
+			     enum dma_data_direction direction)
+{
+	struct udmabuf *ubuf = buf->priv;
+	struct device *dev = ubuf->udmabuf_misc->this_device;
+
+	if (!ubuf->sg) {
+		ubuf->sg = get_sg_table(dev, buf, direction);
+		if (IS_ERR(ubuf->sg))
+			return PTR_ERR(ubuf->sg);
+	} else {
+		dma_sync_sg_for_device(dev, ubuf->sg->sgl,
+				       ubuf->sg->nents,
+				       direction);
+	}
+
+	return 0;
+}
+
+static int end_cpu_udmabuf(struct dma_buf *buf,
+			   enum dma_data_direction direction)
+{
+	struct udmabuf *ubuf = buf->priv;
+	struct device *dev = ubuf->udmabuf_misc->this_device;
+
+	if (!ubuf->sg)
+		return -EINVAL;
+
+	dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents, direction);
+	return 0;
+}
+
 static const struct dma_buf_ops udmabuf_ops = {
 	.cache_sgt_mapping = true,
 	.map_dma_buf	   = map_udmabuf,
@@ -136,6 +173,8 @@ static const struct dma_buf_ops udmabuf_ops = {
 	.map		   = kmap_udmabuf,
 	.unmap		   = kunmap_udmabuf,
 	.mmap		   = mmap_udmabuf,
+	.begin_cpu_access  = begin_cpu_udmabuf,
+	.end_cpu_access    = end_cpu_udmabuf,
 };
 
 #define SEALS_WANTED (F_SEAL_SHRINK)
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 2/6] udmabuf: add ability to set access flags on udmabuf

[Gerd Hoffmann]

On Wed, Jul 31, 2019 at 07:25:13PM -0700, Gurchetan Singh wrote:
> The main use for udmabuf is sending guest memory pages
> to the host.
> 
> It's generally a bad idea to have to separate mappings with
> different attributes. For example, a WC mapping the guest
> kernel and cached mapping on the host is problematic.
> 
> Add creation time flags so the user has responsibility for
> the specific use case.

> -#define UDMABUF_FLAGS_CLOEXEC	0x01
> +#define UDMABUF_FLAGS_CLOEXEC    0x01
> +#define UDMABUF_FLAGS_PROT_NONE  0x02
> +#define UDMABUF_FLAGS_PROT_READ  0x04
> +#define UDMABUF_FLAGS_PROT_WRITE 0x08

[ didn't look at followup patches yet ]

You can't have readonly/writeonly dmabufs.
So that isn't going to fly.

The commit message suggests this is for cache attributes not protection,
so having the flags might make sense, but please don't name the flags
PROT_* then.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 4/6] udmabuf: add a pointer to the miscdevice in dma-buf private data

[Gerd Hoffmann]

On Wed, Jul 31, 2019 at 07:25:15PM -0700, Gurchetan Singh wrote:
> Will be used later.
> 
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> ---
>  drivers/dma-buf/udmabuf.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 134e53d24c2b..47003abbf4c2 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -19,6 +19,7 @@ struct udmabuf {
>  	u32 flags;
>  	pgoff_t pagecount;
>  	struct page **pages;
> +	struct miscdevice *udmabuf_misc;

I'd name this "dev" or "device".

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 3/6] udmabuf: enforce access flags

[Gerd Hoffmann]

>  static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma)
>  {
>  	struct udmabuf *ubuf = buf->priv;
> +	pgprot_t pgprot = vm_get_page_prot(vma->vm_flags);
>  
>  	if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0)
>  		return -EINVAL;
>  
> +	if (ubuf->flags & UDMABUF_FLAGS_PROT_NONE)
> +		return -EINVAL;
> +
> +	if ((ubuf->flags & UDMABUF_FLAGS_PROT_READ) == 0)

"if ((vma->vm_flags & VM_READ) == 0)" ?

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 2/6] udmabuf: add ability to set access flags on udmabuf

[Gurchetan Singh]

On Wed, Jul 31, 2019 at 11:40 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Wed, Jul 31, 2019 at 07:25:13PM -0700, Gurchetan Singh wrote:
> > The main use for udmabuf is sending guest memory pages
> > to the host.
> >
> > It's generally a bad idea to have to separate mappings with
> > different attributes. For example, a WC mapping the guest
> > kernel and cached mapping on the host is problematic.
> >
> > Add creation time flags so the user has responsibility for
> > the specific use case.
>
> > -#define UDMABUF_FLAGS_CLOEXEC        0x01
> > +#define UDMABUF_FLAGS_CLOEXEC    0x01
> > +#define UDMABUF_FLAGS_PROT_NONE  0x02
> > +#define UDMABUF_FLAGS_PROT_READ  0x04
> > +#define UDMABUF_FLAGS_PROT_WRITE 0x08
>
> [ didn't look at followup patches yet ]
>
> You can't have readonly/writeonly dmabufs.
> So that isn't going to fly.
>
> The commit message suggests this is for cache attributes not protection,
> so having the flags might make sense, but please don't name the flags
> PROT_* then.

Okay, I'll change the flags to CACHED / UNCACHED / WRITE_COMBINE (like
msm_drm.h).  And since the dma api doesn't work on x86 [1], we'll have
to call drm_cflush_pages in the guest.  Since caching is privileged on
ARM and not on x86, that *should* get us write-combine guest buffers.

[1] https://lists.freedesktop.org/archives/dri-devel/2019-August/229161.html

>
> cheers,
>   Gerd
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 2/6] udmabuf: add ability to set access flags on udmabuf

[Gerd Hoffmann]

On Fri, Aug 02, 2019 at 09:45:15AM -0700, Gurchetan Singh wrote:
> On Wed, Jul 31, 2019 at 11:40 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Wed, Jul 31, 2019 at 07:25:13PM -0700, Gurchetan Singh wrote:
> > > The main use for udmabuf is sending guest memory pages
> > > to the host.
> > >
> > > It's generally a bad idea to have to separate mappings with
> > > different attributes. For example, a WC mapping the guest
> > > kernel and cached mapping on the host is problematic.
> > >
> > > Add creation time flags so the user has responsibility for
> > > the specific use case.
> >
> > > -#define UDMABUF_FLAGS_CLOEXEC        0x01
> > > +#define UDMABUF_FLAGS_CLOEXEC    0x01
> > > +#define UDMABUF_FLAGS_PROT_NONE  0x02
> > > +#define UDMABUF_FLAGS_PROT_READ  0x04
> > > +#define UDMABUF_FLAGS_PROT_WRITE 0x08
> >
> > [ didn't look at followup patches yet ]
> >
> > You can't have readonly/writeonly dmabufs.
> > So that isn't going to fly.
> >
> > The commit message suggests this is for cache attributes not protection,
> > so having the flags might make sense, but please don't name the flags
> > PROT_* then.
> 
> Okay, I'll change the flags to CACHED / UNCACHED / WRITE_COMBINE (like
> msm_drm.h).  And since the dma api doesn't work on x86 [1], we'll have
> to call drm_cflush_pages in the guest.  Since caching is privileged on
> ARM and not on x86, that *should* get us write-combine guest buffers.
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2019-August/229161.html

Ah, so you are aware of the vgem cache synchronization patches.

It's probably a good idea to wait until that is finally settled before
following with udmabuf.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel