Re: [patches] Re: [PATCH 16/17] RISC-V: User-facing API

[Palmer Dabbelt]

On Tue, 11 Jul 2017 07:01:32 PDT (-0700), james.hogan@xxxxxxxxxx wrote:
> Hi Christoph,
>
> On Tue, Jul 11, 2017 at 06:39:48AM -0700, Christoph Hellwig wrote:
>> > +#ifdef CONFIG_64BIT
>> > +SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
>> > +	unsigned long, prot, unsigned long, flags,
>> > +	unsigned long, fd, off_t, offset)
>> > +{
>> > +	if (unlikely(offset & (~PAGE_MASK)))
>> > +		return -EINVAL;
>> > +	return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
>> > +}
>> > +#else
>> > +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
>> > +	unsigned long, prot, unsigned long, flags,
>> > +	unsigned long, fd, off_t, offset)
>> > +{
>> > +	/*
>> > +	 * Note that the shift for mmap2 is constant (12),
>> > +	 * regardless of PAGE_SIZE
>> > +	 */
>> > +	if (unlikely(offset & (~PAGE_MASK >> 12)))
>> > +		return -EINVAL;
>> > +	return sys_mmap_pgoff(addr, len, prot, flags, fd,
>> > +		offset >> (PAGE_SHIFT - 12));
>> > +}
>> > +#endif /* !CONFIG_64BIT */
>>
>> Most modern ports seem to expose sys_mmap_pgoff as the
>> syscall directly.  Any reason you're doing this differently?
>
> I think Palmer's patch is probably correct here. Exposing sys_mmap_pgoff
> is only really correct on 32-bit arches where the only page size is 4k.
> If other page sizes are supported then this is the correct way to handle
> it as the page offset from 32-bit userland is supposed to be in 4k
> units.
>
> 64-bit doesn't need to worry about squeezing big file offsets into the
> off_t offset so don't need to do the shift at all.
>
> See the mmap2 man page. It says "the final argument specifies the offset
> into the file in 4096-byte units", and it points out ia64 as an
> exception where it depends on the page size of the system.
>
>>
>> But even the code for the older ones should probably be consolidated..
>
> Quite probably, yes.

This looks like what arm64 does, though I'm OK either way.  Here's my attempt
at consolidating the code, even though there isn't a lot to help with:

  diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
  index e18fc0ebdd91..4351be7d0533 100644
  --- a/arch/riscv/kernel/sys_riscv.c
  +++ b/arch/riscv/kernel/sys_riscv.c
  @@ -17,14 +17,23 @@
   #include <asm/cmpxchg.h>
   #include <asm/unistd.h>

  +static long riscv_sys_mmap(unsigned long addr, unsigned long len,
  +                          unsigned long prot, unsigned long flags,
  +                          unsigned long fd, off_t offset,
  +                          unsigned long page_shift_offset)
  +{
  +       if (unlikely(offset & (~PAGE_MASK >> page_shift_offset)))
  +               return -EINVAL;
  +       return sys_mmap_pgoff(addr, len, prot, flags, fd,
  +                             offset >> (PAGE_SHIFT - page_shift_offset));
  +}
  +
   #ifdef CONFIG_64BIT
   SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
          unsigned long, prot, unsigned long, flags,
          unsigned long, fd, off_t, offset)
   {
  -       if (unlikely(offset & (~PAGE_MASK)))
  -               return -EINVAL;
  -       return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
  +       return riscv_sys_mmap(addr, len, prot, flags, fd, offset, 0);
   }
   #else
   SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
  @@ -35,9 +44,6 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
           * Note that the shift for mmap2 is constant (12),
           * regardless of PAGE_SIZE
           */
  -       if (unlikely(offset & (~PAGE_MASK >> 12)))
  -               return -EINVAL;
  -       return sys_mmap_pgoff(addr, len, prot, flags, fd,
  -               offset >> (PAGE_SHIFT - 12));
  +       return riscv_sys_mmap(addr, len, prot, flags, fd, offset, 12);
   }
   #endif /* !CONFIG_64BIT */

I'll submit this as part of our v6, which will hopefully be coming out soon.

Thanks!