On Sun, Mar 06, 2022 at 07:03:00PM +0200, Jarkko Sakkinen wrote:
> On Sun, Mar 06, 2022 at 11:01:36AM +0100, Greg Kroah-Hartman wrote:
> > On Sun, Mar 06, 2022 at 07:32:05AM +0200, Jarkko Sakkinen wrote:
> > > 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 <jarkko@xxxxxxxxxx>
> > > v3:
> > > - 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:
> > > - 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(-)
> > >
> > > diff --git a/arch/mips/kernel/vdso.c b/arch/mips/kernel/vdso.c
> > > index 3d0cf471f2fe..89f3f3da9abd 100644
> > > --- a/arch/mips/kernel/vdso.c
> > > +++ b/arch/mips/kernel/vdso.c
> > > @@ -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;
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > > index 1b526039a60d..4c71f64d6a79 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > > @@ -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;
> > >
> > > diff --git a/fs/coda/file.c b/fs/coda/file.c
> > > index 29dd87be2fb8..e14f312fdbf8 100644
> > > --- a/fs/coda/file.c
> > > +++ b/fs/coda/file.c
> > > @@ -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
> > > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > > index fa125feed0ff..b963a9397e80 100644
> > > --- a/fs/overlayfs/file.c
> > > +++ b/fs/overlayfs/file.c
> > > @@ -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);
> > >
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index e2d892b201b0..2909e2d14af8 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -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 *);
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index 213cc569b192..6c8c036f423b 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -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.
>
> I can create:
>
> * mmap_region_populate()
> * call_mmap_populate()
>
> This would localize the changes and leave out those boolean parameters.
>
> > 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
~~~~~
intention
BR, Jarkko
