MAP_POPULATE for device memory

Sometimes you might want to use MAP_POPULATE to ask a device driver to initialize the device memory in some specific manner. SGX driver can use this to request more memory by issuing ENCLS[EAUG] x86 opcode for each page in the address range.

Add f_ops->populate() with the same parameters as f_ops->mmap() and make it conditionally called inside call_mmap(). Update call sites accodingly.
---

Signed-off-by Jarkko Sakkinen <[email protected]>

v3:

include/linux/fs.h +2 -1

-       if (!ret && do_populate && file->f_op->populate)
+       if (!ret && do_populate && file->f_op->populate &&
+           !!(vma->vm_flags & (VM_IO | VM_PFNMAP)))

(reported by Matthew Wilcox)
v2:

(quoted diff) +1 -1

-       if (!ret && do_populate)
+       if (!ret && do_populate && file->f_op->populate)

(reported by Jan Harkes)

---
 arch/mips/kernel/vdso.c                    |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  2 +-
 fs/coda/file.c                             |  2 +-
 fs/overlayfs/file.c                        |  2 +-
 include/linux/fs.h                         | 12 ++++++++++--
 include/linux/mm.h                         |  2 +-
 ipc/shm.c                                  |  2 +-
 mm/mmap.c                                  | 10 +++++-----
 mm/nommu.c                                 |  4 ++--
 9 files changed, 23 insertions(+), 15 deletions(-)

arch/mips/kernel/vdso.c +1 -1

index 3d0cf471f2fe..89f3f3da9abd 100644
@@ -102,7 +102,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
 		base = mmap_region(NULL, STACK_TOP, PAGE_SIZE,
 				VM_READ | VM_EXEC |
 				VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC,
-				0, NULL);
+				0, NULL, false);
 		if (IS_ERR_VALUE(base)) {
 			ret = base;
 			goto out;

drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +1 -1

index 1b526039a60d..4c71f64d6a79 100644
@@ -107,7 +107,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *
 	if (!obj->base.filp)
 		return -ENODEV;
 
-	ret = call_mmap(obj->base.filp, vma);
+	ret = call_mmap(obj->base.filp, vma, false);
 	if (ret)
 		return ret;
 

fs/coda/file.c +1 -1

index 29dd87be2fb8..e14f312fdbf8 100644
@@ -173,7 +173,7 @@ coda_file_mmap(struct file *coda_file, struct vm_area_struct *vma)
 	spin_unlock(&cii->c_lock);
 
 	vma->vm_file = get_file(host_file);
-	ret = call_mmap(vma->vm_file, vma);
+	ret = call_mmap(vma->vm_file, vma, false);
 
 	if (ret) {
 		/* if call_mmap fails, our caller will put host_file so we

fs/overlayfs/file.c +1 -1

index fa125feed0ff..b963a9397e80 100644
@@ -503,7 +503,7 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
 	vma_set_file(vma, realfile);
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
-	ret = call_mmap(vma->vm_file, vma);
+	ret = call_mmap(vma->vm_file, vma, false);
 	revert_creds(old_cred);
 	ovl_file_accessed(file);
 

include/linux/fs.h +10 -2

index e2d892b201b0..2909e2d14af8 100644
@@ -42,6 +42,7 @@
 #include <linux/mount.h>
 #include <linux/cred.h>
 #include <linux/mnt_idmapping.h>
+#include <linux/mm.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>

@@ -1993,6 +1994,7 @@ struct file_operations {
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
+	int (*populate)(struct file *, struct vm_area_struct *);
 	unsigned long mmap_supported_flags;
 	int (*open) (struct inode *, struct file *);
 	int (*flush) (struct file *, fl_owner_t id);

@@ -2074,9 +2076,15 @@ static inline ssize_t call_write_iter(struct file *file, struct kiocb *kio,
 	return file->f_op->write_iter(kio, iter);
 }
 
-static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
+static inline int call_mmap(struct file *file, struct vm_area_struct *vma, bool do_populate)
 {
-	return file->f_op->mmap(file, vma);
+	int ret = file->f_op->mmap(file, vma);
+
+	if (!ret && do_populate && file->f_op->populate &&
+	    !!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
+		ret = file->f_op->populate(file, vma);
+
+	return ret;
 }
 
 extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);

include/linux/mm.h +0 -0

index 213cc569b192..6c8c036f423b 100644

Quoted 126 lines · Jarkko Sakkinen

Sometimes you might want to use MAP_POPULATE to ask a device driver to initialize the device memory in some specific manner. SGX driver can use this to request more memory by issuing ENCLS[EAUG] x86 opcode for each page in the address range.

Add f_ops->populate() with the same parameters as f_ops->mmap() and make it conditionally called inside call_mmap(). Update call sites accodingly.
---

Signed-off-by Jarkko Sakkinen <[email protected]>

v3:

include/linux/fs.h +2 -1

-       if (!ret && do_populate && file->f_op->populate)
+       if (!ret && do_populate && file->f_op->populate &&
+           !!(vma->vm_flags & (VM_IO | VM_PFNMAP)))

(reported by Matthew Wilcox)
v2:

(quoted diff) +1 -1

-       if (!ret && do_populate)
+       if (!ret && do_populate && file->f_op->populate)

(reported by Jan Harkes)

---
 arch/mips/kernel/vdso.c                    |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  2 +-
 fs/coda/file.c                             |  2 +-
 fs/overlayfs/file.c                        |  2 +-
 include/linux/fs.h                         | 12 ++++++++++--
 include/linux/mm.h                         |  2 +-
 ipc/shm.c                                  |  2 +-
 mm/mmap.c                                  | 10 +++++-----
 mm/nommu.c                                 |  4 ++--
 9 files changed, 23 insertions(+), 15 deletions(-)

arch/mips/kernel/vdso.c +1 -1

index 3d0cf471f2fe..89f3f3da9abd 100644
@@ -102,7 +102,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
 		base = mmap_region(NULL, STACK_TOP, PAGE_SIZE,
 				VM_READ | VM_EXEC |
 				VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC,
-				0, NULL);
+				0, NULL, false);
 		if (IS_ERR_VALUE(base)) {
 			ret = base;
 			goto out;

drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +1 -1

index 1b526039a60d..4c71f64d6a79 100644
@@ -107,7 +107,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *
 	if (!obj->base.filp)
 		return -ENODEV;
 
-	ret = call_mmap(obj->base.filp, vma);
+	ret = call_mmap(obj->base.filp, vma, false);
 	if (ret)
 		return ret;
 

fs/coda/file.c +1 -1

index 29dd87be2fb8..e14f312fdbf8 100644
@@ -173,7 +173,7 @@ coda_file_mmap(struct file *coda_file, struct vm_area_struct *vma)
 	spin_unlock(&cii->c_lock);
 
 	vma->vm_file = get_file(host_file);
-	ret = call_mmap(vma->vm_file, vma);
+	ret = call_mmap(vma->vm_file, vma, false);
 
 	if (ret) {
 		/* if call_mmap fails, our caller will put host_file so we

fs/overlayfs/file.c +1 -1

index fa125feed0ff..b963a9397e80 100644
@@ -503,7 +503,7 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
 	vma_set_file(vma, realfile);
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
-	ret = call_mmap(vma->vm_file, vma);
+	ret = call_mmap(vma->vm_file, vma, false);
 	revert_creds(old_cred);
 	ovl_file_accessed(file);
 

include/linux/fs.h +10 -2

index e2d892b201b0..2909e2d14af8 100644
@@ -42,6 +42,7 @@
 #include <linux/mount.h>
 #include <linux/cred.h>
 #include <linux/mnt_idmapping.h>
+#include <linux/mm.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>

@@ -1993,6 +1994,7 @@ struct file_operations {
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
+	int (*populate)(struct file *, struct vm_area_struct *);
 	unsigned long mmap_supported_flags;
 	int (*open) (struct inode *, struct file *);
 	int (*flush) (struct file *, fl_owner_t id);

@@ -2074,9 +2076,15 @@ static inline ssize_t call_write_iter(struct file *file, struct kiocb *kio,
 	return file->f_op->write_iter(kio, iter);
 }
 
-static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
+static inline int call_mmap(struct file *file, struct vm_area_struct *vma, bool do_populate)
 {
-	return file->f_op->mmap(file, vma);
+	int ret = file->f_op->mmap(file, vma);
+
+	if (!ret && do_populate && file->f_op->populate &&
+	    !!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
+		ret = file->f_op->populate(file, vma);
+
+	return ret;
 }
 
 extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);

include/linux/mm.h +0 -0

index 213cc569b192..6c8c036f423b 100644

include/linux/mm.h extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned lo · line 2686

Quoted 5 lines

include/linux/mm.h +1 -1

@@ -2683,7 +2683,7 @@ extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned lo
 
 extern unsigned long mmap_region(struct file *file, unsigned long addr,
 	unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
-	struct list_head *uf);
+	struct list_head *uf, bool do_populate);

As I have said many times before, don't add random boolean flags to function arguments, as they provide no hint as to what they do at all. When you read the code, you then have to go back and look up the function definition here and see what exactly it means and the flow is broken.

Make function names mean something obvious, for this, if it really is a good idea to have this new flag (and I doubt it, but that's not my call), then make this a mmap_region_populate() call to make it obvious it is something different than the notmal mmap_region() call.

But as is, this is pretty horrid, don't you agree?

thanks,

greg k-h

Quoted 139 lines · Greg Kroah-Hartman

Quoted 126 lines · Jarkko Sakkinen

Sometimes you might want to use MAP_POPULATE to ask a device driver to initialize the device memory in some specific manner. SGX driver can use this to request more memory by issuing ENCLS[EAUG] x86 opcode for each page in the address range.

Add f_ops->populate() with the same parameters as f_ops->mmap() and make it conditionally called inside call_mmap(). Update call sites accodingly.
---

Signed-off-by Jarkko Sakkinen <[email protected]>

v3:

include/linux/fs.h +2 -1

-       if (!ret && do_populate && file->f_op->populate)
+       if (!ret && do_populate && file->f_op->populate &&
+           !!(vma->vm_flags & (VM_IO | VM_PFNMAP)))

(reported by Matthew Wilcox)
v2:

(quoted diff) +1 -1

-       if (!ret && do_populate)
+       if (!ret && do_populate && file->f_op->populate)

(reported by Jan Harkes)

---
 arch/mips/kernel/vdso.c                    |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  2 +-
 fs/coda/file.c                             |  2 +-
 fs/overlayfs/file.c                        |  2 +-
 include/linux/fs.h                         | 12 ++++++++++--
 include/linux/mm.h                         |  2 +-
 ipc/shm.c                                  |  2 +-
 mm/mmap.c                                  | 10 +++++-----
 mm/nommu.c                                 |  4 ++--
 9 files changed, 23 insertions(+), 15 deletions(-)

arch/mips/kernel/vdso.c +1 -1

index 3d0cf471f2fe..89f3f3da9abd 100644
@@ -102,7 +102,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
 		base = mmap_region(NULL, STACK_TOP, PAGE_SIZE,
 				VM_READ | VM_EXEC |
 				VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC,
-				0, NULL);
+				0, NULL, false);
 		if (IS_ERR_VALUE(base)) {
 			ret = base;
 			goto out;

drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +1 -1

index 1b526039a60d..4c71f64d6a79 100644
@@ -107,7 +107,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *
 	if (!obj->base.filp)
 		return -ENODEV;
 
-	ret = call_mmap(obj->base.filp, vma);
+	ret = call_mmap(obj->base.filp, vma, false);
 	if (ret)
 		return ret;
 

fs/coda/file.c +1 -1

index 29dd87be2fb8..e14f312fdbf8 100644
@@ -173,7 +173,7 @@ coda_file_mmap(struct file *coda_file, struct vm_area_struct *vma)
 	spin_unlock(&cii->c_lock);
 
 	vma->vm_file = get_file(host_file);
-	ret = call_mmap(vma->vm_file, vma);
+	ret = call_mmap(vma->vm_file, vma, false);
 
 	if (ret) {
 		/* if call_mmap fails, our caller will put host_file so we

fs/overlayfs/file.c +1 -1

index fa125feed0ff..b963a9397e80 100644
@@ -503,7 +503,7 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
 	vma_set_file(vma, realfile);
 
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
-	ret = call_mmap(vma->vm_file, vma);
+	ret = call_mmap(vma->vm_file, vma, false);
 	revert_creds(old_cred);
 	ovl_file_accessed(file);
 

include/linux/fs.h +10 -2

index e2d892b201b0..2909e2d14af8 100644
@@ -42,6 +42,7 @@
 #include <linux/mount.h>
 #include <linux/cred.h>
 #include <linux/mnt_idmapping.h>
+#include <linux/mm.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>

@@ -1993,6 +1994,7 @@ struct file_operations {
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
 	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
 	int (*mmap) (struct file *, struct vm_area_struct *);
+	int (*populate)(struct file *, struct vm_area_struct *);
 	unsigned long mmap_supported_flags;
 	int (*open) (struct inode *, struct file *);
 	int (*flush) (struct file *, fl_owner_t id);

@@ -2074,9 +2076,15 @@ static inline ssize_t call_write_iter(struct file *file, struct kiocb *kio,
 	return file->f_op->write_iter(kio, iter);
 }
 
-static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
+static inline int call_mmap(struct file *file, struct vm_area_struct *vma, bool do_populate)
 {
-	return file->f_op->mmap(file, vma);
+	int ret = file->f_op->mmap(file, vma);
+
+	if (!ret && do_populate && file->f_op->populate &&
+	    !!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
+		ret = file->f_op->populate(file, vma);
+
+	return ret;
 }
 
 extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);

include/linux/mm.h +0 -0

index 213cc569b192..6c8c036f423b 100644

include/linux/mm.h extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned lo · line 2686

Quoted 5 lines

include/linux/mm.h +1 -1

@@ -2683,7 +2683,7 @@ extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned lo
 
 extern unsigned long mmap_region(struct file *file, unsigned long addr,
 	unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
-	struct list_head *uf);
+	struct list_head *uf, bool do_populate);

As I have said many times before, don't add random boolean flags to function arguments, as they provide no hint as to what they do at all. When you read the code, you then have to go back and look up the function definition here and see what exactly it means and the flow is broken.

Quoted 4 lines

Make function names mean something obvious, for this, if it really is a good idea to have this new flag (and I doubt it, but that's not my call), then make this a mmap_region_populate() call to make it obvious it is something different than the notmal mmap_region() call.

I can create:

* mmap_region_populate()
* call_mmap_populate()

This would localize the changes and leave out those boolean parameters.

Quoted 1 line

But as is, this is pretty horrid, don't you agree?

So can I conclude from this that in general having populate available for device memory is something horrid, or just the implementation path?

That's the main reason why I made this RFC patch set, to get clear answer to that question. I.e. if it is in general sense a bad idea, I'll just create ioctl. If it is the implementation, I'll try to improve it.

Otherwise, I don't know whether or not it is good idea to include such patch into the main SGX2 patch set. No means enforcibl tryy to push support

So can I conclude from this that in general having populate available for device memory is something horrid, or just the implementation path?

Quoted 2 lines · Jarkko Sakkinen

So can I conclude from this that in general having populate available for device memory is something horrid, or just the implementation path?

You haven't even attempted to explain what the problem is you're trying to solve. You've shown up with some terrible code and said "Hey, is this a good idea". No, no, it's not.

Quoted 7 lines · Matthew Wilcox

Quoted 2 lines · Jarkko Sakkinen

So can I conclude from this that in general having populate available for device memory is something horrid, or just the implementation path?

You haven't even attempted to explain what the problem is you're trying to solve. You've shown up with some terrible code and said "Hey, is this a good idea". No, no, it's not.

The problem is that in order to include memory to enclave, which is essentially a reserved address range processes virtual address space there's two steps into it:

1. Host side (kernel) does ENCLS[EAUG] to request a new page to be added to the enclave.
2. Enclave accepts request with ENCLU[EACCEPT] or ENCLU[EACCEPTCOPY].

In the current SGX2 patch set this taken care by the page fault handler. I.e. the enclave calls ENCLU[EACCEPT] for an empty address and the #PF handler then does EAUG for a single page.

So if you want to process a batch of pages this generates O(n) round-trips.

So if there was a way pre-do a batch of EAUG's, that would allow to load data to the enclave without causing page faults happening constantly.

One solution for this simply add ioctl:

https://lore.kernel.org/linux-sgx/[email protected]/T/#m195ec84bf85614a140abeee245c5118c22ace8f3

But in practice when you wanted to use it, you would setup the parameters so that they match the mmap() range. So for pratical user space API having mmap() take care of this would be much more lean option.

Sent: 06 March 2022 05:32

For device memory (aka VM_IO | VM_PFNMAP) MAP_POPULATE does nothing. Allow to use that for initializing the device memory by providing a new callback f_ops->populate() for the purpose.

SGX patches are provided to show the callback in context.

An obvious alternative is a ioctl but it is less elegant and requires two syscalls (mmap + ioctl) per memory range, instead of just one (mmap).

From: Jarkko Sakkinen

Quoted 6 lines

Sent: 06 March 2022 05:32

For device memory (aka VM_IO | VM_PFNMAP) MAP_POPULATE does nothing. Allow to use that for initializing the device memory by providing a new callback f_ops->populate() for the purpose.

Quoted 5 lines

SGX patches are provided to show the callback in context.

An obvious alternative is a ioctl but it is less elegant and requires two syscalls (mmap + ioctl) per memory range, instead of just one (mmap).

Is this all about trying to stop the vm_operations_struct.fault() function being called?

For device memory (aka VM_IO | VM_PFNMAP) MAP_POPULATE does nothing. Allow to use that for initializing the device memory by providing a new callback f_ops->populate() for the purpose.

Quoted 3 lines · Jarkko Sakkinen

For device memory (aka VM_IO | VM_PFNMAP) MAP_POPULATE does nothing. Allow to use that for initializing the device memory by providing a new callback f_ops->populate() for the purpose.

As I said, NAK.

Quoted 6 lines · Matthew Wilcox

Quoted 3 lines · Jarkko Sakkinen

For device memory (aka VM_IO | VM_PFNMAP) MAP_POPULATE does nothing. Allow to use that for initializing the device memory by providing a new callback f_ops->populate() for the purpose.

As I said, NAK.

Agreed. This is an amazingly bad interface.

So what would you suggest to sort out the issue? I'm happy to go with ioctl if nothing else is acceptable.

Quoted 2 lines · Jarkko Sakkinen

So what would you suggest to sort out the issue? I'm happy to go with ioctl if nothing else is acceptable.

PLenty of drivers treat all mmaps as if MAP_POPULATE was specified, typically by using (io_)remap_pfn_range. If there any reason to only optionally have the pre-fault semantics for sgx? If not this should be really simple. And if we have a real need for it to be optional we'll just need to find a sane way to pass that information to ->mmap.

Sent: 07 March 2022 15:57

Quoted 2 lines · Jarkko Sakkinen

So what would you suggest to sort out the issue? I'm happy to go with ioctl if nothing else is acceptable.

PLenty of drivers treat all mmaps as if MAP_POPULATE was specified, typically by using (io_)remap_pfn_range. If there any reason to only optionally have the pre-fault semantics for sgx? If not this should be really simple. And if we have a real need for it to be optional we'll just need to find a sane way to pass that information to ->mmap.

From: Christoph Hellwig

Quoted 11 lines

Sent: 07 March 2022 15:57

Quoted 2 lines · Jarkko Sakkinen

So what would you suggest to sort out the issue? I'm happy to go with ioctl if nothing else is acceptable.

PLenty of drivers treat all mmaps as if MAP_POPULATE was specified, typically by using (io_)remap_pfn_range. If there any reason to only optionally have the pre-fault semantics for sgx? If not this should be really simple. And if we have a real need for it to be optional we'll just need to find a sane way to pass that information to ->mmap.

Is there any space in vma->vm_flags ?

That would be better than an extra argument or function.

For device memory (aka VM_IO | VM_PFNMAP) MAP_POPULATE does nothing. Allow to use that for initializing the device memory by providing a new callback f_ops->populate() for the purpose.

SGX patches are provided to show the callback in context.

An obvious alternative is a ioctl but it is less elegant and requires two syscalls (mmap + ioctl) per memory range, instead of just one (mmap).

Quoted 4 lines · Jarkko Sakkinen

For device memory (aka VM_IO | VM_PFNMAP) MAP_POPULATE does nothing. Allow to use that for initializing the device memory by providing a new callback f_ops->populate() for the purpose.

Quoted 5 lines

SGX patches are provided to show the callback in context.

An obvious alternative is a ioctl but it is less elegant and requires two syscalls (mmap + ioctl) per memory range, instead of just one (mmap).

What about extending MADV_POPULATE_READ | MADV_POPULATE_WRITE to support VM_IO | VM_PFNMAP (as well?) ?

Signature · 3 lines

Thanks,

David / dhildenb

Quoted 11 lines · David Hildenbrand

Quoted 9 lines · Jarkko Sakkinen

For device memory (aka VM_IO | VM_PFNMAP) MAP_POPULATE does nothing. Allow to use that for initializing the device memory by providing a new callback f_ops->populate() for the purpose.

SGX patches are provided to show the callback in context.

An obvious alternative is a ioctl but it is less elegant and requires two syscalls (mmap + ioctl) per memory range, instead of just one (mmap).

Quoted 2 lines

What about extending MADV_POPULATE_READ | MADV_POPULATE_WRITE to support VM_IO | VM_PFNMAP (as well?) ?

What would be a proper point to bind that behaviour? For mmap/mprotect it'd be probably populate_vma_page_range() because that would span both mmap() and mprotect() (Dave's suggestion in this thread).

For MAP_POPULATE I did not have hard proof to show that it would be used by other drivers but for madvice() you can find at least a few ioctl based implementations:

$ git grep -e madv --and \( -e ioc \)  drivers/
drivers/gpu/drm/i915/gem/i915_gem_ioctls.h:int i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
drivers/gpu/drm/i915/i915_driver.c:     DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_RENDER_ALLOW),
drivers/gpu/drm/i915/i915_gem.c:i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
drivers/gpu/drm/msm/msm_drv.c:static int msm_ioctl_gem_madvise(struct drm_device *dev, void *data,
drivers/gpu/drm/msm/msm_drv.c:  DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  DRM_RENDER_ALLOW),
drivers/gpu/drm/panfrost/panfrost_drv.c:static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
drivers/gpu/drm/vc4/vc4_drv.c:  DRM_IOCTL_DEF_DRV(VC4_GEM_MADVISE, vc4_gem_madvise_ioctl, DRM_RENDER_ALLOW),
drivers/gpu/drm/vc4/vc4_drv.h:int vc4_gem_madvise_ioctl(struct drm_device *dev, void *data,
drivers/gpu/drm/vc4/vc4_gem.c:int vc4_gem_madvise_ioctl(struct drm_device *dev, void *data,

IMHO this also provides supportive claim for MAP_POPULATE, and yeah, I agree that to be consistent implementation, both madvice() and MAP_POPULATE should work.

Quoted 12 lines · Jarkko Sakkinen

Quoted 11 lines · David Hildenbrand

Quoted 9 lines · Jarkko Sakkinen

For device memory (aka VM_IO | VM_PFNMAP) MAP_POPULATE does nothing. Allow to use that for initializing the device memory by providing a new callback f_ops->populate() for the purpose.

SGX patches are provided to show the callback in context.

An obvious alternative is a ioctl but it is less elegant and requires two syscalls (mmap + ioctl) per memory range, instead of just one (mmap).

Quoted 6 lines

Quoted 2 lines

What about extending MADV_POPULATE_READ | MADV_POPULATE_WRITE to support VM_IO | VM_PFNMAP (as well?) ?

What would be a proper point to bind that behaviour? For mmap/mprotect it'd be probably populate_vma_page_range() because that would span both mmap() and mprotect() (Dave's suggestion in this thread).

MADV_POPULATE_* ends up in faultin_vma_page_range(), right next to populate_vma_page_range(). So it might require a similar way to hook into the driver I guess.

Quoted 16 lines

For MAP_POPULATE I did not have hard proof to show that it would be used by other drivers but for madvice() you can find at least a few ioctl based implementations:

$ git grep -e madv --and \( -e ioc \)  drivers/
drivers/gpu/drm/i915/gem/i915_gem_ioctls.h:int i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
drivers/gpu/drm/i915/i915_driver.c:     DRM_IOCTL_DEF_DRV(I915_GEM_MADVISE, i915_gem_madvise_ioctl, DRM_RENDER_ALLOW),
drivers/gpu/drm/i915/i915_gem.c:i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
drivers/gpu/drm/msm/msm_drv.c:static int msm_ioctl_gem_madvise(struct drm_device *dev, void *data,
drivers/gpu/drm/msm/msm_drv.c:  DRM_IOCTL_DEF_DRV(MSM_GEM_MADVISE,  msm_ioctl_gem_madvise,  DRM_RENDER_ALLOW),
drivers/gpu/drm/panfrost/panfrost_drv.c:static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
drivers/gpu/drm/vc4/vc4_drv.c:  DRM_IOCTL_DEF_DRV(VC4_GEM_MADVISE, vc4_gem_madvise_ioctl, DRM_RENDER_ALLOW),
drivers/gpu/drm/vc4/vc4_drv.h:int vc4_gem_madvise_ioctl(struct drm_device *dev, void *data,
drivers/gpu/drm/vc4/vc4_gem.c:int vc4_gem_madvise_ioctl(struct drm_device *dev, void *data,

Quoted 3 lines

IMHO this also provides supportive claim for MAP_POPULATE, and yeah, I agree that to be consistent implementation, both madvice() and MAP_POPULATE should work.

MADV_POPULATE_WRITE + MADV_DONTNEED/FALLOC_FL_PUNCH_HOLE is one way to dynamically manage memory consumption inside a sparse memory mapping (preallocate/populate via MADV_POPULATE_WRITE, discard via MADV_DONTNEED/FALLOC_FL_PUNCH_HOLE). Extending that whole mechanism to deal with VM_IO | VM_PFNMAP mappings as well could be interesting.

At least I herd about some ideas where we might want to dynamically expose memory to a VM (via virtio-mem) inside a sparse memory mapping, and the memory in that sparse memory mapping is provided from a dedicated memory pool managed by a device driver -- not just using ordinary anonymous/file/hugetlb memory as we do right now.

Now, this is certainly stuff for the future, I just wanted to mention it.

Signature · 3 lines

Thanks,

David / dhildenb